diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index 92ad3c5..216c6c9 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -1025,16 +1025,21 @@ defmodule Mv.Membership.Member do authorize_if expr(id == ^actor(:member_id)) end - # 2. GENERAL: Check permissions from role - # - :own_data → can UPDATE linked member (scope :linked via HasPermission) - # - :read_only → can READ all members (scope :all), no update permission - # - :normal_user → can CRUD all members (scope :all) - # - :admin → can CRUD all members (scope :all) - policy action_type([:read, :create, :update, :destroy]) do + # 2. READ/DESTROY: Check permissions only (no :user argument on these actions) + policy action_type([:read, :destroy]) do description "Check permissions from user's role" authorize_if Mv.Authorization.Checks.HasPermission end - + + # 3. CREATE/UPDATE: Forbid user link unless admin; then check permissions + # ForbidMemberUserLinkUnlessAdmin: only admins may pass :user (link or unlink via nil/empty). + # HasPermission: :own_data → update linked; :read_only → no update; :normal_user/admin → update all + policy action_type([:create, :update]) do + description "Forbid user link unless admin; then check permissions" + forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin + authorize_if Mv.Authorization.Checks.HasPermission + end + # 4. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) end @@ -1054,6 +1059,8 @@ end - **READ list queries**: No record at strict_check time → bypass with `expr(id == ^actor(:member_id))` needed for auto_filter ✅ - **UPDATE operations**: Changeset contains record → HasPermission evaluates `scope :linked` correctly ✅ +**User–member link:** Only admins may pass the `:user` argument on create_member or update_member (link or unlink via `user: nil`/`user: %{}`). The check uses **argument presence** (key in arguments), not value, to avoid bypass (see [User-Member Linking](#user-member-linking)). + **Permission Matrix:** | Action | Mitglied | Vorstand | Kassenwart | Buchhaltung | Admin | @@ -1148,23 +1155,20 @@ end **Location:** `lib/mv/authorization/role.ex` -**Special Protection:** System roles cannot be deleted. +**Defense-in-depth:** The Role resource uses `authorizers: [Ash.Policy.Authorizer]` and policies with `Mv.Authorization.Checks.HasPermission`. **Read** is allowed for all permission sets (own_data, read_only, normal_user, admin) via `perm("Role", :read, :all)` in PermissionSets; reading roles is not a security concern. **Create, update, and destroy** are allowed only for admin (admin has full Role CRUD in PermissionSets). Seeds and bootstrap use `authorize?: false` where necessary. + +**Special Protection:** System roles cannot be deleted (validation on destroy). ```elixir defmodule Mv.Authorization.Role do - use Ash.Resource, ... + use Ash.Resource, + authorizers: [Ash.Policy.Authorizer] policies do - # Only admin can manage roles policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from user's role" + description "Check permissions from user's role (read all, create/update/destroy admin only)" authorize_if Mv.Authorization.Checks.HasPermission end - - # DEFAULT: Forbid - policy action_type([:read, :create, :update, :destroy]) do - forbid_if always() - end end # Prevent deletion of system roles @@ -1201,7 +1205,7 @@ end | Action | Mitglied | Vorstand | Kassenwart | Buchhaltung | Admin | |--------|----------|----------|------------|-------------|-------| -| Read | ❌ | ❌ | ❌ | ❌ | ✅ | +| Read | ✅ | ✅ | ✅ | ✅ | ✅ | | Create | ❌ | ❌ | ❌ | ❌ | ✅ | | Update | ❌ | ❌ | ❌ | ❌ | ✅ | | Destroy* | ❌ | ❌ | ❌ | ❌ | ✅ | @@ -2045,7 +2049,10 @@ Users and Members are separate entities that can be linked. Special rules: - A user cannot link themselves to an existing member - A user CAN create a new member and be directly linked to it (self-service) -**Enforcement:** The User resource restricts the `update_user` action (which accepts the `member` argument for link/unlink) to admins only via `Mv.Authorization.Checks.ActorIsAdmin`. The UserLive.Form shows the Member-Linking UI and runs member link/unlink on save only when the current user is admin; non-admins use the `:update` action (email only) for profile edit. +**Enforcement:** + +- **User side:** The User resource restricts the `update_user` action (which accepts the `member` argument for link/unlink) to admins only via `Mv.Authorization.Checks.ActorIsAdmin`. The UserLive.Form shows the Member-Linking UI and runs member link/unlink on save only when the current user is admin; non-admins use the `:update` action (email only) for profile edit. +- **Member side:** Only admins may set or change the user–member link on **Member** create or update. When creating or updating a member, the `:user` argument (which links the member to a user account) is forbidden for non-admins. This is enforced by `Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin` in the Member resource policies (`forbid_if` before `authorize_if HasPermission`). Non-admins can still create and update members as long as they do **not** pass the `:user` argument. The Member resource uses **`on_missing: :ignore`** for the `:user` relationship on update_member, so **omitting** `:user` from params does **not** change the link (no "unlink by omission"); unlink is only possible by explicitly passing `:user` (e.g. `user: nil`), which is admin-only. ### Approach: Separate Ash Actions diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 8213ecb..476501c 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -153,16 +153,18 @@ defmodule Mv.Membership.Member do change manage_relationship(:custom_field_values, on_match: :update, on_no_match: :create) + # When :user argument is present and nil/empty, unrelate (admin-only via policy). + # Must run before manage_relationship; on_missing: :ignore then does nothing for nil input. + change Mv.Membership.Member.Changes.UnrelateUserWhenArgumentNil + # Manage the user relationship during member update + # on_missing: :ignore so that omitting :user does NOT unlink (security: only admins may + # change the link; unlink is explicit via user: nil, forbidden for non-admins by policy). change manage_relationship(:user, :user, - # Look up existing user and relate to it on_lookup: :relate, - # Error if user doesn't exist in database on_no_match: :error, - # Error if user is already linked to another member (prevents "stealing") on_match: :error, - # If no user provided, remove existing relationship (allows user removal) - on_missing: :unrelate + on_missing: :ignore ) # Sync member email to user when email changes (Member → User) @@ -312,14 +314,18 @@ defmodule Mv.Membership.Member do 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" + # READ/DESTROY: Check permissions only (no :user argument on these actions) + policy action_type([:read, :destroy]) do + description "Check permissions from user's role" + authorize_if Mv.Authorization.Checks.HasPermission + end + + # CREATE/UPDATE: Forbid member–user link unless admin, then check permissions + # ForbidMemberUserLinkUnlessAdmin: only admins may pass :user (link or unlink via nil/empty). + # HasPermission: :own_data → update linked; :read_only → no update; :normal_user/admin → update all. + policy action_type([:create, :update]) do + description "Forbid user link unless admin; then check permissions" + forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin authorize_if Mv.Authorization.Checks.HasPermission end diff --git a/lib/membership/member/changes/unrelate_user_when_argument_nil.ex b/lib/membership/member/changes/unrelate_user_when_argument_nil.ex new file mode 100644 index 0000000..dc4d097 --- /dev/null +++ b/lib/membership/member/changes/unrelate_user_when_argument_nil.ex @@ -0,0 +1,50 @@ +defmodule Mv.Membership.Member.Changes.UnrelateUserWhenArgumentNil do + @moduledoc """ + When :user argument is present and nil/empty on update_member, unrelate the current user. + + With on_missing: :ignore, manage_relationship does not unrelate when input is nil/[]. + This change handles explicit unlink (user: nil or user: %{}) by updating the linked + User to set member_id = nil. Only runs when the argument key is present (policy + ForbidMemberUserLinkUnlessAdmin ensures only admins can pass :user). + """ + use Ash.Resource.Change + + @spec change(Ash.Changeset.t(), keyword(), Ash.Resource.Change.context()) :: Ash.Changeset.t() + def change(changeset, _opts, _context) do + if unlink_requested?(changeset) do + unrelate_current_user(changeset) + else + changeset + end + end + + defp unlink_requested?(changeset) do + args = changeset.arguments || %{} + + if Map.has_key?(args, :user) or Map.has_key?(args, "user") do + user_arg = Ash.Changeset.get_argument(changeset, :user) + user_arg == nil or (is_map(user_arg) and map_size(user_arg) == 0) + else + false + end + end + + defp unrelate_current_user(changeset) do + member = changeset.data + actor = Map.get(changeset.context || %{}, :actor) + + case Ash.load(member, :user, domain: Mv.Membership, authorize?: false) do + {:ok, %{user: user}} when not is_nil(user) -> + # User's :update action only accepts [:email]; use :update_user so + # manage_relationship(:member, ..., on_missing: :unrelate) runs and clears member_id. + user + |> Ash.Changeset.for_update(:update_user, %{member: nil}, domain: Mv.Accounts) + |> Ash.update(domain: Mv.Accounts, actor: actor, authorize?: false) + + changeset + + _ -> + changeset + end + end +end diff --git a/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex b/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex new file mode 100644 index 0000000..1e7cb77 --- /dev/null +++ b/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex @@ -0,0 +1,71 @@ +defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do + @moduledoc """ + Policy check: forbids setting or changing the member–user link unless the actor is admin. + + Used on Member create_member and update_member actions. When the `:user` argument + **is present** (key in arguments, regardless of value), only admins may perform the action. + This covers: + - **Linking:** `user: %{id: user_id}` → only admin + - **Unlinking:** explicit `user: nil` or `user: %{}` on update_member → only admin + Non-admin users can create and update members only when they do **not** pass the + `:user` argument; omitting `:user` leaves the relationship unchanged. + + ## Unlink semantics (update_member) + + The Member resource uses `on_missing: :ignore` for the `:user` relationship on update. + So **omitting** `:user` from params does **not** change the link (no "unlink by omission"). + Unlink is only possible by **explicitly** passing `:user` (e.g. `user: nil`), which this + check forbids for non-admins. Admins may link or unlink via the `:user` argument. + + ## Usage + + In Member resource policies, restrict to create/update only: + + policy action_type([:create, :update]) do + forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin + authorize_if Mv.Authorization.Checks.HasPermission + end + + ## Behaviour + + - If the `:user` argument **key is not present** → does not forbid. + - If `:user` is present (any value, including nil or %{}) and actor is not admin → forbids. + - If actor is nil → treated as non-admin (forbid when :user present). `Actor.admin?(nil)` is defined and returns false. + - If actor is admin (or system actor) → does not forbid. + """ + use Ash.Policy.Check + + alias Mv.Authorization.Actor + + @impl true + def describe(_opts), do: "forbid setting member–user link unless actor is admin" + + @impl true + def strict_check(actor, authorizer, _opts) do + # Nil actor: treat as non-admin (Actor.admin?(nil) returns false; no crash) + actor = if is_nil(actor), do: nil, else: Actor.ensure_loaded(actor) + + if user_argument_present?(authorizer) and not Actor.admin?(actor) do + {:ok, true} + else + {:ok, false} + end + end + + # Forbid when :user was passed at all (link, unlink via nil/empty, or invalid value). + # Check argument key presence (atom or string) for defense-in-depth. + defp user_argument_present?(authorizer) do + args = get_arguments(authorizer) || %{} + Map.has_key?(args, :user) or Map.has_key?(args, "user") + end + + defp get_arguments(authorizer) do + subject = authorizer.changeset || authorizer.subject + + cond do + is_struct(subject, Ash.Changeset) -> subject.arguments + is_struct(subject, Ash.ActionInput) -> subject.arguments + true -> %{} + end + end +end diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index 9a5f7a7..b0e7015 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/lib/mv/authorization/permission_sets.ex @@ -78,6 +78,7 @@ defmodule Mv.Authorization.PermissionSets do defp custom_field_read_all, do: [perm("CustomField", :read, :all)] defp membership_fee_type_read_all, do: [perm("MembershipFeeType", :read, :all)] defp membership_fee_cycle_read_all, do: [perm("MembershipFeeCycle", :read, :all)] + defp role_read_all, do: [perm("Role", :read, :all)] @doc """ Returns the list of all valid permission set names. @@ -129,7 +130,8 @@ defmodule Mv.Authorization.PermissionSets do group_read_all() ++ [perm("MemberGroup", :read, :linked)] ++ membership_fee_type_read_all() ++ - [perm("MembershipFeeCycle", :read, :linked)], + [perm("MembershipFeeCycle", :read, :linked)] ++ + role_read_all(), pages: [ # No "/" - Mitglied must not see member index at root (same content as /members). # Own profile (sidebar links to /users/:id) and own user edit @@ -156,7 +158,8 @@ defmodule Mv.Authorization.PermissionSets do group_read_all() ++ [perm("MemberGroup", :read, :all)] ++ membership_fee_type_read_all() ++ - membership_fee_cycle_read_all(), + membership_fee_cycle_read_all() ++ + role_read_all(), pages: [ "/", # Own profile (sidebar links to /users/:id; redirect target must be allowed) @@ -211,7 +214,8 @@ defmodule Mv.Authorization.PermissionSets do perm("MembershipFeeCycle", :create, :all), perm("MembershipFeeCycle", :update, :all), perm("MembershipFeeCycle", :destroy, :all) - ], + ] ++ + role_read_all(), pages: [ "/", # Own profile (sidebar links to /users/:id; redirect target must be allowed) diff --git a/lib/mv/authorization/role.ex b/lib/mv/authorization/role.ex index 9c33e2d..59c0e51 100644 --- a/lib/mv/authorization/role.ex +++ b/lib/mv/authorization/role.ex @@ -37,7 +37,8 @@ defmodule Mv.Authorization.Role do """ use Ash.Resource, domain: Mv.Authorization, - data_layer: AshPostgres.DataLayer + data_layer: AshPostgres.DataLayer, + authorizers: [Ash.Policy.Authorizer] postgres do table "roles" @@ -86,6 +87,13 @@ defmodule Mv.Authorization.Role do end end + policies do + policy action_type([:read, :create, :update, :destroy]) do + description "Role access: read for all permission sets, create/update/destroy for admin only (PermissionSets)" + authorize_if Mv.Authorization.Checks.HasPermission + end + end + validations do validate one_of( :permission_set_name, diff --git a/test/mv/authorization/role_policies_test.exs b/test/mv/authorization/role_policies_test.exs new file mode 100644 index 0000000..449f9d6 --- /dev/null +++ b/test/mv/authorization/role_policies_test.exs @@ -0,0 +1,226 @@ +defmodule Mv.Authorization.RolePoliciesTest do + @moduledoc """ + Tests for Role resource authorization policies. + + Rule: All permission sets (own_data, read_only, normal_user, admin) can **read** roles. + Only **admin** can create, update, or destroy roles. + """ + use Mv.DataCase, async: false + + alias Mv.Authorization + alias Mv.Authorization.Role + + describe "read access - all permission sets can read roles" do + setup do + # Create a role to read (via system_actor; once policies exist, system_actor is admin) + role = Mv.Fixtures.role_fixture("read_only") + %{role: role} + end + + @tag :permission_set_own_data + test "own_data can list roles", %{role: _role} do + user = Mv.Fixtures.user_with_role_fixture("own_data") + user = Mv.Authorization.Actor.ensure_loaded(user) + + {:ok, roles} = Authorization.list_roles(actor: user) + + assert is_list(roles) + assert roles != [] + end + + @tag :permission_set_own_data + test "own_data can get role by id", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("own_data") + user = Mv.Authorization.Actor.ensure_loaded(user) + + {:ok, loaded} = Ash.get(Role, role.id, actor: user, domain: Mv.Authorization) + + assert loaded.id == role.id + end + + @tag :permission_set_read_only + test "read_only can list roles", %{role: _role} do + user = Mv.Fixtures.user_with_role_fixture("read_only") + user = Mv.Authorization.Actor.ensure_loaded(user) + + {:ok, roles} = Authorization.list_roles(actor: user) + + assert is_list(roles) + assert roles != [] + end + + @tag :permission_set_read_only + test "read_only can get role by id", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("read_only") + user = Mv.Authorization.Actor.ensure_loaded(user) + + {:ok, loaded} = Ash.get(Role, role.id, actor: user, domain: Mv.Authorization) + + assert loaded.id == role.id + end + + @tag :permission_set_normal_user + test "normal_user can list roles", %{role: _role} do + user = Mv.Fixtures.user_with_role_fixture("normal_user") + user = Mv.Authorization.Actor.ensure_loaded(user) + + {:ok, roles} = Authorization.list_roles(actor: user) + + assert is_list(roles) + assert roles != [] + end + + @tag :permission_set_normal_user + test "normal_user can get role by id", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("normal_user") + user = Mv.Authorization.Actor.ensure_loaded(user) + + {:ok, loaded} = Ash.get(Role, role.id, actor: user, domain: Mv.Authorization) + + assert loaded.id == role.id + end + + @tag :permission_set_admin + test "admin can list roles", %{role: _role} do + admin = Mv.Fixtures.user_with_role_fixture("admin") + admin = Mv.Authorization.Actor.ensure_loaded(admin) + + {:ok, roles} = Authorization.list_roles(actor: admin) + + assert is_list(roles) + assert roles != [] + end + + @tag :permission_set_admin + test "admin can get role by id", %{role: role} do + admin = Mv.Fixtures.user_with_role_fixture("admin") + admin = Mv.Authorization.Actor.ensure_loaded(admin) + + {:ok, loaded} = Ash.get(Role, role.id, actor: admin, domain: Mv.Authorization) + + assert loaded.id == role.id + end + end + + describe "create/update/destroy - only admin allowed" do + setup do + # Non-system role for destroy test (role_fixture creates non-system roles) + role = Mv.Fixtures.role_fixture("normal_user") + %{role: role} + end + + test "admin can create_role", %{role: _role} do + admin = Mv.Fixtures.user_with_role_fixture("admin") + admin = Mv.Authorization.Actor.ensure_loaded(admin) + + attrs = %{ + name: "New Role #{System.unique_integer([:positive])}", + description: "Test", + permission_set_name: "read_only" + } + + assert {:ok, _created} = Authorization.create_role(attrs, actor: admin) + end + + test "admin can update_role", %{role: role} do + admin = Mv.Fixtures.user_with_role_fixture("admin") + admin = Mv.Authorization.Actor.ensure_loaded(admin) + + assert {:ok, updated} = + Authorization.update_role(role, %{description: "Updated by admin"}, actor: admin) + + assert updated.description == "Updated by admin" + end + + test "admin can destroy non-system role", %{role: role} do + admin = Mv.Fixtures.user_with_role_fixture("admin") + admin = Mv.Authorization.Actor.ensure_loaded(admin) + + assert :ok = Authorization.destroy_role(role, actor: admin) + end + + test "own_data cannot create_role (forbidden)", %{role: _role} do + user = Mv.Fixtures.user_with_role_fixture("own_data") + user = Mv.Authorization.Actor.ensure_loaded(user) + + attrs = %{ + name: "New Role #{System.unique_integer([:positive])}", + description: "Test", + permission_set_name: "read_only" + } + + assert {:error, %Ash.Error.Forbidden{}} = Authorization.create_role(attrs, actor: user) + end + + test "own_data cannot update_role (forbidden)", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("own_data") + user = Mv.Authorization.Actor.ensure_loaded(user) + + assert {:error, %Ash.Error.Forbidden{}} = + Authorization.update_role(role, %{description: "Updated"}, actor: user) + end + + test "own_data cannot destroy_role (forbidden)", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("own_data") + user = Mv.Authorization.Actor.ensure_loaded(user) + + assert {:error, %Ash.Error.Forbidden{}} = Authorization.destroy_role(role, actor: user) + end + + test "read_only cannot create_role (forbidden)", %{role: _role} do + user = Mv.Fixtures.user_with_role_fixture("read_only") + user = Mv.Authorization.Actor.ensure_loaded(user) + + attrs = %{ + name: "New Role #{System.unique_integer([:positive])}", + description: "Test", + permission_set_name: "read_only" + } + + assert {:error, %Ash.Error.Forbidden{}} = Authorization.create_role(attrs, actor: user) + end + + test "read_only cannot update_role (forbidden)", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("read_only") + user = Mv.Authorization.Actor.ensure_loaded(user) + + assert {:error, %Ash.Error.Forbidden{}} = + Authorization.update_role(role, %{description: "Updated"}, actor: user) + end + + test "read_only cannot destroy_role (forbidden)", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("read_only") + user = Mv.Authorization.Actor.ensure_loaded(user) + + assert {:error, %Ash.Error.Forbidden{}} = Authorization.destroy_role(role, actor: user) + end + + test "normal_user cannot create_role (forbidden)", %{role: _role} do + user = Mv.Fixtures.user_with_role_fixture("normal_user") + user = Mv.Authorization.Actor.ensure_loaded(user) + + attrs = %{ + name: "New Role #{System.unique_integer([:positive])}", + description: "Test", + permission_set_name: "normal_user" + } + + assert {:error, %Ash.Error.Forbidden{}} = Authorization.create_role(attrs, actor: user) + end + + test "normal_user cannot update_role (forbidden)", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("normal_user") + user = Mv.Authorization.Actor.ensure_loaded(user) + + assert {:error, %Ash.Error.Forbidden{}} = + Authorization.update_role(role, %{description: "Updated"}, actor: user) + end + + test "normal_user cannot destroy_role (forbidden)", %{role: role} do + user = Mv.Fixtures.user_with_role_fixture("normal_user") + user = Mv.Authorization.Actor.ensure_loaded(user) + + assert {:error, %Ash.Error.Forbidden{}} = Authorization.destroy_role(role, actor: user) + end + end +end diff --git a/test/mv/authorization/role_test.exs b/test/mv/authorization/role_test.exs index b7aa632..426719a 100644 --- a/test/mv/authorization/role_test.exs +++ b/test/mv/authorization/role_test.exs @@ -12,27 +12,29 @@ defmodule Mv.Authorization.RoleTest do end describe "permission_set_name validation" do - test "accepts valid permission set names" do + test "accepts valid permission set names", %{actor: actor} do attrs = %{ name: "Test Role", permission_set_name: "own_data" } - assert {:ok, role} = Authorization.create_role(attrs) + assert {:ok, role} = Authorization.create_role(attrs, actor: actor) assert role.permission_set_name == "own_data" end - test "rejects invalid permission set names" do + test "rejects invalid permission set names", %{actor: actor} do attrs = %{ name: "Test Role", permission_set_name: "invalid_set" } - assert {:error, %Ash.Error.Invalid{errors: errors}} = Authorization.create_role(attrs) + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Authorization.create_role(attrs, actor: actor) + assert error_message(errors, :permission_set_name) =~ "must be one of" end - test "accepts all four valid permission sets" do + test "accepts all four valid permission sets", %{actor: actor} do valid_sets = ["own_data", "read_only", "normal_user", "admin"] for permission_set <- valid_sets do @@ -41,7 +43,7 @@ defmodule Mv.Authorization.RoleTest do permission_set_name: permission_set } - assert {:ok, _role} = Authorization.create_role(attrs) + assert {:ok, _role} = Authorization.create_role(attrs, actor: actor) end end end @@ -60,34 +62,36 @@ defmodule Mv.Authorization.RoleTest do {:ok, system_role} = Ash.create(changeset, actor: actor) assert {:error, %Ash.Error.Invalid{errors: errors}} = - Authorization.destroy_role(system_role) + Authorization.destroy_role(system_role, actor: actor) message = error_message(errors, :is_system_role) assert message =~ "Cannot delete system role" end - test "allows deletion of non-system roles" do + test "allows deletion of non-system roles", %{actor: actor} do # is_system_role defaults to false, so regular create works {:ok, regular_role} = - Authorization.create_role(%{ - name: "Regular Role", - permission_set_name: "read_only" - }) + Authorization.create_role( + %{name: "Regular Role", permission_set_name: "read_only"}, + actor: actor + ) - assert :ok = Authorization.destroy_role(regular_role) + assert :ok = Authorization.destroy_role(regular_role, actor: actor) end end describe "name uniqueness" do - test "enforces unique role names" do + test "enforces unique role names", %{actor: actor} do attrs = %{ name: "Unique Role", permission_set_name: "own_data" } - assert {:ok, _} = Authorization.create_role(attrs) + assert {:ok, _} = Authorization.create_role(attrs, actor: actor) + + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Authorization.create_role(attrs, actor: actor) - assert {:error, %Ash.Error.Invalid{errors: errors}} = Authorization.create_role(attrs) assert error_message(errors, :name) =~ "has already been taken" end end diff --git a/test/mv/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs index c2715ae..add2ad5 100644 --- a/test/mv/helpers/system_actor_test.exs +++ b/test/mv/helpers/system_actor_test.exs @@ -18,18 +18,21 @@ defmodule Mv.Helpers.SystemActorTest do Ecto.Adapters.SQL.query!(Mv.Repo, "DELETE FROM users WHERE id = $1", [id]) end - # Helper function to ensure admin role exists + # Helper function to ensure admin role exists (bootstrap: no actor yet, use authorize?: false) defp ensure_admin_role do - case Authorization.list_roles() do + case Authorization.list_roles(authorize?: false) do {:ok, roles} -> case Enum.find(roles, &(&1.permission_set_name == "admin")) do nil -> {:ok, role} = - Authorization.create_role(%{ - name: "Admin", - description: "Administrator with full access", - permission_set_name: "admin" - }) + Authorization.create_role( + %{ + name: "Admin", + description: "Administrator with full access", + permission_set_name: "admin" + }, + authorize?: false + ) role @@ -39,11 +42,14 @@ defmodule Mv.Helpers.SystemActorTest do _ -> {:ok, role} = - Authorization.create_role(%{ - name: "Admin", - description: "Administrator with full access", - permission_set_name: "admin" - }) + Authorization.create_role( + %{ + name: "Admin", + description: "Administrator with full access", + permission_set_name: "admin" + }, + authorize?: false + ) role end @@ -364,12 +370,17 @@ defmodule Mv.Helpers.SystemActorTest do test "raises error if system user has wrong role", %{system_user: system_user} do # Create a non-admin role (using read_only as it's a valid permission set) + system_actor = SystemActor.get_system_actor() + {:ok, read_only_role} = - Authorization.create_role(%{ - name: "Read Only Role", - description: "Read-only access", - permission_set_name: "read_only" - }) + Authorization.create_role( + %{ + name: "Read Only Role", + description: "Read-only access", + permission_set_name: "read_only" + }, + actor: system_actor + ) system_actor = SystemActor.get_system_actor() diff --git a/test/mv/membership/member_policies_test.exs b/test/mv/membership/member_policies_test.exs index a66941b..f2d3084 100644 --- a/test/mv/membership/member_policies_test.exs +++ b/test/mv/membership/member_policies_test.exs @@ -403,4 +403,184 @@ defmodule Mv.Membership.MemberPoliciesTest do assert updated_member.first_name == "Updated" end end + + describe "member user link - only admin may set or change user link" do + setup %{actor: actor} do + normal_user = + Mv.Fixtures.user_with_role_fixture("normal_user") + |> Mv.Authorization.Actor.ensure_loaded() + + admin = + Mv.Fixtures.user_with_role_fixture("admin") + |> Mv.Authorization.Actor.ensure_loaded() + + unlinked_member = create_unlinked_member(actor) + + %{normal_user: normal_user, admin: admin, unlinked_member: unlinked_member} + end + + test "normal_user can create member without :user argument", %{normal_user: normal_user} do + {:ok, member} = + Membership.create_member( + %{ + first_name: "NoLink", + last_name: "Member", + email: "nolink#{System.unique_integer([:positive])}@example.com" + }, + actor: normal_user + ) + + assert member.first_name == "NoLink" + # Member has_one :user (FK on User side); ensure no user is linked + {:ok, member} = + Ash.load(member, :user, domain: Mv.Membership, actor: normal_user) + + assert is_nil(member.user) + end + + test "normal_user cannot create member with :user argument (forbidden)", %{ + normal_user: normal_user + } do + other_user = + Mv.Fixtures.user_with_role_fixture("read_only") + |> Mv.Authorization.Actor.ensure_loaded() + + attrs = %{ + first_name: "Linked", + last_name: "Member", + email: "linked#{System.unique_integer([:positive])}@example.com", + user: %{id: other_user.id} + } + + assert {:error, %Ash.Error.Forbidden{}} = + Membership.create_member(attrs, actor: normal_user) + end + + test "normal_user can update member without :user argument", %{ + normal_user: normal_user, + unlinked_member: unlinked_member + } do + {:ok, updated} = + Membership.update_member(unlinked_member, %{first_name: "UpdatedByNormal"}, + actor: normal_user + ) + + assert updated.first_name == "UpdatedByNormal" + end + + test "normal_user cannot update member with :user argument (forbidden)", %{ + normal_user: normal_user, + unlinked_member: unlinked_member + } do + other_user = + Mv.Fixtures.user_with_role_fixture("own_data") + |> Mv.Authorization.Actor.ensure_loaded() + + params = %{first_name: unlinked_member.first_name, user: %{id: other_user.id}} + + assert {:error, %Ash.Error.Forbidden{}} = + Membership.update_member(unlinked_member, params, actor: normal_user) + end + + test "normal_user cannot update member with user: nil (unlink forbidden)", %{ + normal_user: normal_user, + unlinked_member: unlinked_member + } do + # Link member first (via admin), then normal_user tries to unlink via user: nil + admin = + Mv.Fixtures.user_with_role_fixture("admin") |> Mv.Authorization.Actor.ensure_loaded() + + link_target = + Mv.Fixtures.user_with_role_fixture("own_data") |> Mv.Authorization.Actor.ensure_loaded() + + {:ok, linked_member} = + Membership.update_member( + unlinked_member, + %{user: %{id: link_target.id}}, + actor: admin + ) + + # Passing user: nil explicitly tries to unlink; only admin may do that + assert {:error, %Ash.Error.Forbidden{}} = + Membership.update_member(linked_member, %{user: nil}, actor: normal_user) + end + + test "normal_user update linked member without :user keeps link", %{ + normal_user: normal_user, + admin: admin, + unlinked_member: unlinked_member + } do + # Admin links member to a user + link_target = + Mv.Fixtures.user_with_role_fixture("own_data") + |> Mv.Authorization.Actor.ensure_loaded() + + {:ok, linked_member} = + Membership.update_member( + unlinked_member, + %{user: %{id: link_target.id}}, + actor: admin + ) + + # normal_user updates only first_name (no :user) – link must remain (on_missing: :ignore) + {:ok, updated} = + Membership.update_member(linked_member, %{first_name: "Updated"}, actor: normal_user) + + assert updated.first_name == "Updated" + + {:ok, user} = + Ash.get(Mv.Accounts.User, link_target.id, domain: Mv.Accounts, actor: admin) + + assert user.member_id == updated.id + end + + test "admin can create member with :user argument", %{admin: admin} do + link_target = + Mv.Fixtures.user_with_role_fixture("own_data") + |> Mv.Authorization.Actor.ensure_loaded() + + attrs = %{ + first_name: "AdminLinked", + last_name: "Member", + email: "adminlinked#{System.unique_integer([:positive])}@example.com", + user: %{id: link_target.id} + } + + {:ok, member} = Membership.create_member(attrs, actor: admin) + + assert member.first_name == "AdminLinked" + + {:ok, link_target} = + Ash.get(Mv.Accounts.User, link_target.id, domain: Mv.Accounts, actor: admin) + + assert link_target.member_id == member.id + end + + test "admin can update member with :user argument (link)", %{ + admin: admin, + unlinked_member: unlinked_member + } do + link_target = + Mv.Fixtures.user_with_role_fixture("read_only") + |> Mv.Authorization.Actor.ensure_loaded() + + {:ok, updated} = + Membership.update_member( + unlinked_member, + %{user: %{id: link_target.id}}, + actor: admin + ) + + assert updated.id == unlinked_member.id + + {:ok, reloaded_user} = + Ash.get(Mv.Accounts.User, link_target.id, + domain: Mv.Accounts, + load: [:member], + actor: admin + ) + + assert reloaded_user.member_id == updated.id + end + end end diff --git a/test/mv_web/authorization_test.exs b/test/mv_web/authorization_test.exs index d07e482..7bb0b2a 100644 --- a/test/mv_web/authorization_test.exs +++ b/test/mv_web/authorization_test.exs @@ -50,14 +50,14 @@ defmodule MvWeb.AuthorizationTest do assert Authorization.can?(admin, :destroy, Mv.Authorization.Role) == true end - test "non-admin cannot manage roles" do + test "non-admin can read roles but cannot create/update/destroy" do normal_user = %{ id: "normal-123", role: %{permission_set_name: "normal_user"} } + assert Authorization.can?(normal_user, :read, Mv.Authorization.Role) == true assert Authorization.can?(normal_user, :create, Mv.Authorization.Role) == false - assert Authorization.can?(normal_user, :read, Mv.Authorization.Role) == false assert Authorization.can?(normal_user, :update, Mv.Authorization.Role) == false assert Authorization.can?(normal_user, :destroy, Mv.Authorization.Role) == false end diff --git a/test/mv_web/live/role_live/show_test.exs b/test/mv_web/live/role_live/show_test.exs index ed099ec..fe5c48d 100644 --- a/test/mv_web/live/role_live/show_test.exs +++ b/test/mv_web/live/role_live/show_test.exs @@ -18,7 +18,7 @@ defmodule MvWeb.RoleLive.ShowTest do alias Mv.Authorization alias Mv.Authorization.Role - # Helper to create a role + # Helper to create a role (authorize?: false for test data setup) defp create_role(attrs \\ %{}) do default_attrs = %{ name: "Test Role #{System.unique_integer([:positive])}", @@ -28,7 +28,7 @@ defmodule MvWeb.RoleLive.ShowTest do attrs = Map.merge(default_attrs, attrs) - case Authorization.create_role(attrs) do + case Authorization.create_role(attrs, authorize?: false) do {:ok, role} -> role {:error, error} -> raise "Failed to create role: #{inspect(error)}" end @@ -38,7 +38,7 @@ defmodule MvWeb.RoleLive.ShowTest do defp create_admin_user(conn, actor) do # Create admin role admin_role = - case Authorization.list_roles() do + case Authorization.list_roles(authorize?: false) do {:ok, roles} -> case Enum.find(roles, &(&1.name == "Admin")) do nil -> diff --git a/test/mv_web/live/role_live_test.exs b/test/mv_web/live/role_live_test.exs index 0edd2a4..cb112f2 100644 --- a/test/mv_web/live/role_live_test.exs +++ b/test/mv_web/live/role_live_test.exs @@ -9,7 +9,7 @@ defmodule MvWeb.RoleLiveTest do alias Mv.Authorization alias Mv.Authorization.Role - # Helper to create a role + # Helper to create a role (authorize?: false for test data setup) defp create_role(attrs \\ %{}) do default_attrs = %{ name: "Test Role #{System.unique_integer([:positive])}", @@ -19,7 +19,7 @@ defmodule MvWeb.RoleLiveTest do attrs = Map.merge(default_attrs, attrs) - case Authorization.create_role(attrs) do + case Authorization.create_role(attrs, authorize?: false) do {:ok, role} -> role {:error, error} -> raise "Failed to create role: #{inspect(error)}" end @@ -29,7 +29,7 @@ defmodule MvWeb.RoleLiveTest do defp create_admin_user(conn, actor) do # Create admin role admin_role = - case Authorization.list_roles() do + case Authorization.list_roles(authorize?: false) do {:ok, roles} -> case Enum.find(roles, &(&1.name == "Admin")) do nil -> @@ -332,7 +332,7 @@ defmodule MvWeb.RoleLiveTest do assert match?({:error, {:redirect, %{to: "/admin/roles"}}}, result) end - test "updates role name", %{conn: conn, role: role} do + test "updates role name", %{conn: conn, role: role, actor: actor} do {:ok, view, _html} = live(conn, "/admin/roles/#{role.id}/edit?return_to=show") attrs = %{ @@ -348,7 +348,7 @@ defmodule MvWeb.RoleLiveTest do assert_redirect(view, "/admin/roles/#{role.id}") # Verify update - {:ok, updated_role} = Authorization.get_role(role.id) + {:ok, updated_role} = Authorization.get_role(role.id, actor: actor) assert updated_role.name == "Updated Role Name" end @@ -377,7 +377,7 @@ defmodule MvWeb.RoleLiveTest do assert_redirect(view, "/admin/roles/#{system_role.id}") # Verify update - {:ok, updated_role} = Authorization.get_role(system_role.id) + {:ok, updated_role} = Authorization.get_role(system_role.id, actor: actor) assert updated_role.permission_set_name == "read_only" end end @@ -390,7 +390,7 @@ defmodule MvWeb.RoleLiveTest do end @tag :slow - test "deletes non-system role", %{conn: conn} do + test "deletes non-system role", %{conn: conn, actor: actor} do role = create_role() {:ok, view, html} = live(conn, "/admin/roles") @@ -404,7 +404,7 @@ defmodule MvWeb.RoleLiveTest do # Verify deletion by checking database assert {:error, %Ash.Error.Invalid{errors: [%Ash.Error.Query.NotFound{}]}} = - Authorization.get_role(role.id) + Authorization.get_role(role.id, actor: actor) end test "fails to delete system role with error message", %{conn: conn, actor: actor} do @@ -430,7 +430,7 @@ defmodule MvWeb.RoleLiveTest do assert render(view) =~ "System roles cannot be deleted" # Role should still exist - {:ok, _role} = Authorization.get_role(system_role.id) + {:ok, _role} = Authorization.get_role(system_role.id, actor: actor) end end