diff --git a/config/test.exs b/config/test.exs index 33d608d..fe2b855 100644 --- a/config/test.exs +++ b/config/test.exs @@ -15,7 +15,7 @@ config :mv, Mv.Repo, pool_size: System.schedulers_online() * 8, queue_target: 5000, queue_interval: 1000, - timeout: 30_000 + timeout: 60_000 # We don't run a server during test. If one is required, # you can enable the server option below. diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index badbd72..bcaf506 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -67,6 +67,10 @@ defmodule Mv.Accounts.User do identity_field :email hash_provider AshAuthentication.BcryptProvider confirmation_required? false + + resettable do + sender Mv.Accounts.User.Senders.SendPasswordResetEmail + end end end end @@ -115,6 +119,8 @@ defmodule Mv.Accounts.User do argument :member, :map, allow_nil?: true upsert? true + # Note: Default role is automatically assigned via attribute default (see attributes block) + # Manage the member relationship during user creation change manage_relationship(:member, :member, # Look up existing member and relate to it @@ -239,6 +245,8 @@ defmodule Mv.Accounts.User do upsert? true # Upsert based on oidc_id (primary match for existing OIDC users) upsert_identity :unique_oidc_id + # On upsert, only update email - preserve existing role_id + upsert_fields [:email] validate &__MODULE__.validate_oidc_id_present/2 @@ -261,6 +269,9 @@ defmodule Mv.Accounts.User do # - The LinkOidcAccountLive will auto-link passwordless users without password prompt validate Mv.Accounts.User.Validations.OidcEmailCollision + # Note: Default role is automatically assigned via attribute default (see attributes block) + # upsert_fields [:email] ensures existing users' roles are preserved during upserts + # Sync user email to member when linking (User → Member) change Mv.EmailSync.Changes.SyncUserEmailToMember end @@ -379,6 +390,15 @@ defmodule Mv.Accounts.User do attribute :hashed_password, :string, sensitive?: true, allow_nil?: true attribute :oidc_id, :string, allow_nil?: true + + # Role assignment: Explicitly defined to enforce default value + # This ensures every user has a role, regardless of creation path + # (register_with_password, create_user, seeds, etc.) + attribute :role_id, :uuid do + allow_nil? false + default &__MODULE__.default_role_id/0 + public? false + end end relationships do @@ -388,10 +408,13 @@ defmodule Mv.Accounts.User do belongs_to :member, Mv.Membership.Member # 1:1 relationship - User belongs to a Role - # This automatically creates a `role_id` attribute in the User table - # The relationship is optional (allow_nil? true by default) + # We define role_id ourselves (above in attributes) to control default value # Foreign key constraint: on_delete: :restrict (prevents deleting roles assigned to users) - belongs_to :role, Mv.Authorization.Role + belongs_to :role, Mv.Authorization.Role do + define_attribute? false + source_attribute :role_id + allow_nil? false + end end identities do @@ -411,4 +434,60 @@ defmodule Mv.Accounts.User do # forbid_if(always()) # end # end + + @doc """ + Returns the default role ID for new users. + + This function is called automatically when creating a user without an explicit role_id. + It fetches the "Mitglied" role from the database without authorization checks + (safe during user creation bootstrap phase). + + The result is cached in the process dictionary to avoid repeated database queries + during high-volume user creation. The cache is invalidated on application restart. + + ## Bootstrap Safety + + Only non-nil values are cached. If the role doesn't exist yet (e.g., before seeds run), + `nil` is not cached, allowing subsequent calls to retry after the role is created. + This prevents bootstrap issues where a process would be permanently stuck with `nil` + if the first call happens before the role exists. + + ## Performance Note + + This function makes one database query per process (cached in process dictionary). + For very high-volume scenarios, consider using a fixed UUID from Application config + instead of querying the database. + + ## Returns + + - UUID of the "Mitglied" role if it exists + - `nil` if the role doesn't exist (will cause validation error due to `allow_nil? false`) + + ## Examples + + iex> Mv.Accounts.User.default_role_id() + "019bf2e2-873a-7712-a7ce-a5a1f90c5f4f" + """ + @spec default_role_id() :: Ecto.UUID.t() | nil + def default_role_id do + # Cache in process dictionary to avoid repeated queries + # IMPORTANT: Only cache non-nil values to avoid bootstrap issues. + # If the role doesn't exist yet (e.g., before seeds run), we don't cache nil + # so that subsequent calls can retry after the role is created. + case Process.get({__MODULE__, :default_role_id}) do + nil -> + role_id = + case Mv.Authorization.Role.get_mitglied_role() do + {:ok, %Mv.Authorization.Role{id: id}} -> id + _ -> nil + end + + # Only cache non-nil values to allow retry if role is created later + if role_id, do: Process.put({__MODULE__, :default_role_id}, role_id) + role_id + + cached_role_id -> + cached_role_id + end + end end diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 97b74c0..1a478b8 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -348,12 +348,22 @@ defmodule Mv.Authorization.Checks.HasPermission do "Member" -> # User.member_id → Member.id (inverse relationship) # Filter: member.id == actor.member_id - {:filter, expr(id == ^actor.member_id)} + # If actor has no member_id, return no results (use false or impossible condition) + if is_nil(actor.member_id) do + {:filter, expr(false)} + else + {:filter, expr(id == ^actor.member_id)} + end "CustomFieldValue" -> # CustomFieldValue.member_id → Member.id → User.member_id # Filter: custom_field_value.member_id == actor.member_id - {:filter, expr(member_id == ^actor.member_id)} + # If actor has no member_id, return no results + if is_nil(actor.member_id) do + {:filter, expr(false)} + else + {:filter, expr(member_id == ^actor.member_id)} + end _ -> # Fallback for other resources diff --git a/lib/mv/authorization/role.ex b/lib/mv/authorization/role.ex index da43510..9c33e2d 100644 --- a/lib/mv/authorization/role.ex +++ b/lib/mv/authorization/role.ex @@ -67,6 +67,11 @@ defmodule Mv.Authorization.Role do # Custom validations will still work end + create :create_role_with_system_flag do + description "Internal action to create roles, allowing `is_system_role` to be set. Used by seeds and migrations." + accept [:name, :description, :permission_set_name, :is_system_role] + end + update :update_role do primary? true # is_system_role is intentionally excluded - should only be set via seeds/internal actions @@ -139,4 +144,33 @@ defmodule Mv.Authorization.Role do identities do identity :unique_name, [:name] end + + @doc """ + Loads the "Mitglied" role without authorization (for bootstrap operations). + + This is a helper function to avoid code duplication when loading the default + role in changes, migrations, and test setup. + + ## Returns + + - `{:ok, %Mv.Authorization.Role{}}` - The "Mitglied" role + - `{:ok, nil}` - Role doesn't exist + - `{:error, term()}` - Error during lookup + + ## Examples + + {:ok, mitglied_role} = Mv.Authorization.Role.get_mitglied_role() + # => {:ok, %Mv.Authorization.Role{name: "Mitglied", ...}} + + {:ok, nil} = Mv.Authorization.Role.get_mitglied_role() + # => Role doesn't exist (e.g., in test environment before seeds run) + """ + @spec get_mitglied_role() :: {:ok, t() | nil} | {:error, term()} + def get_mitglied_role do + require Ash.Query + + __MODULE__ + |> Ash.Query.filter(name == "Mitglied") + |> Ash.read_one(authorize?: false, domain: Mv.Authorization) + end end diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index 3e773cb..6cf3f0f 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -33,6 +33,8 @@ defmodule MvWeb.UserLive.Form do """ use MvWeb, :live_view + require Jason + import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] @impl true @@ -325,6 +327,7 @@ defmodule MvWeb.UserLive.Form do @impl true def handle_event("save", %{"user" => user_params}, socket) do actor = current_actor(socket) + # First save the user without member changes case submit_form(socket.assigns.form, user_params, actor) do {:ok, user} -> diff --git a/priv/repo/migrations/20260122231235_assign_mitglied_role_to_existing_users.exs b/priv/repo/migrations/20260122231235_assign_mitglied_role_to_existing_users.exs new file mode 100644 index 0000000..548de8b --- /dev/null +++ b/priv/repo/migrations/20260122231235_assign_mitglied_role_to_existing_users.exs @@ -0,0 +1,60 @@ +defmodule Mv.Repo.Migrations.AssignMitgliedRoleToExistingUsers do + @moduledoc """ + Assigns the "Mitglied" role to all existing users without a role. + + This migration runs once during deployment to ensure all users have a role assigned. + New users will automatically get the "Mitglied" role via the role_id attribute's default function. + """ + use Ecto.Migration + import Ecto.Query + + def up do + # Find or create the "Mitglied" role + # This ensures the migration works even if seeds haven't run yet + mitglied_role_id = + case repo().one( + from(r in "roles", + where: r.name == "Mitglied", + select: r.id + ) + ) do + nil -> + # Role doesn't exist - create it + # This is idempotent and safe because the role name is unique + # Use execute with SQL string to properly use uuid_generate_v7() function + execute(""" + INSERT INTO roles (id, name, description, permission_set_name, is_system_role, inserted_at, updated_at) + VALUES (uuid_generate_v7(), 'Mitglied', 'Default member role with access to own data only', 'own_data', true, (now() AT TIME ZONE 'utc'), (now() AT TIME ZONE 'utc')) + """) + + # Get the created role ID + role_id = + repo().one( + from(r in "roles", + where: r.name == "Mitglied", + select: r.id + ) + ) + + IO.puts("✅ Created 'Mitglied' role (was missing)") + role_id + + role_id -> + role_id + end + + # Assign Mitglied role to all users without a role + {count, _} = + repo().update_all( + from(u in "users", where: is_nil(u.role_id)), + set: [role_id: mitglied_role_id] + ) + + IO.puts("✅ Assigned 'Mitglied' role to #{count} existing user(s)") + end + + def down do + # Not reversible - we can't know which users had no role before + :ok + end +end diff --git a/priv/repo/migrations/20260125155125_add_not_null_constraint_to_users_role_id.exs b/priv/repo/migrations/20260125155125_add_not_null_constraint_to_users_role_id.exs new file mode 100644 index 0000000..0de605d --- /dev/null +++ b/priv/repo/migrations/20260125155125_add_not_null_constraint_to_users_role_id.exs @@ -0,0 +1,36 @@ +defmodule Mv.Repo.Migrations.AddNotNullConstraintToUsersRoleId do + @moduledoc """ + Adds NOT NULL constraint to users.role_id column. + + This ensures that role_id can never be NULL at the database level, + providing an additional safety layer beyond Ash's allow_nil? false. + + Before running this migration, ensure all existing users have a role_id + (the previous migration AssignMitgliedRoleToExistingUsers handles this). + """ + use Ecto.Migration + + def up do + # First ensure all users have a role_id (safety check) + # This should already be done by the previous migration, but we check anyway + execute(""" + UPDATE users + SET role_id = ( + SELECT id FROM roles WHERE name = 'Mitglied' LIMIT 1 + ) + WHERE role_id IS NULL + """) + + # Now add NOT NULL constraint + alter table(:users) do + modify :role_id, :uuid, null: false + end + end + + def down do + # Remove NOT NULL constraint (allow NULL again) + alter table(:users) do + modify :role_id, :uuid, null: true + end + end +end diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 91b6fa3..1a1f80f 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -5,7 +5,6 @@ alias Mv.Membership alias Mv.Accounts -alias Mv.Authorization alias Mv.MembershipFees.MembershipFeeType alias Mv.MembershipFees.CycleGenerator @@ -129,28 +128,79 @@ end # Get admin email from environment variable or use default admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" -# Create admin role (used for assigning to admin users) -admin_role = - case Authorization.list_roles() do - {:ok, roles} -> - case Enum.find(roles, &(&1.name == "Admin" && &1.permission_set_name == "admin")) do - nil -> - # Create admin role if it doesn't exist - case Authorization.create_role(%{ - name: "Admin", - description: "Administrator with full access", - permission_set_name: "admin" - }) do - {:ok, role} -> role - {:error, _error} -> nil - end +# Create all authorization roles (idempotent - creates only if they don't exist) +# Roles are created using create_role_with_system_flag to allow setting is_system_role +role_configs = [ + %{ + name: "Mitglied", + description: "Default member role with access to own data only", + permission_set_name: "own_data", + is_system_role: true + }, + %{ + name: "Vorstand", + description: "Board member with read access to all member data", + permission_set_name: "read_only", + is_system_role: false + }, + %{ + name: "Kassenwart", + description: "Treasurer with full member and payment management", + permission_set_name: "normal_user", + is_system_role: false + }, + %{ + name: "Buchhaltung", + description: "Accounting with read-only access for auditing", + permission_set_name: "read_only", + is_system_role: false + }, + %{ + name: "Admin", + description: "Administrator with unrestricted access", + permission_set_name: "admin", + is_system_role: false + } +] - role -> - role +# Create or update each role +Enum.each(role_configs, fn role_data -> + # Bind role name to variable to avoid issues with ^ pinning in macros + role_name = role_data.name + + case Mv.Authorization.Role + |> Ash.Query.filter(name == ^role_name) + |> Ash.read_one(authorize?: false, domain: Mv.Authorization) do + {:ok, existing_role} when not is_nil(existing_role) -> + # Role exists - update if needed (preserve is_system_role) + if existing_role.permission_set_name != role_data.permission_set_name or + existing_role.description != role_data.description do + existing_role + |> Ash.Changeset.for_update(:update_role, %{ + description: role_data.description, + permission_set_name: role_data.permission_set_name + }) + |> Ash.update!(authorize?: false, domain: Mv.Authorization) end - {:error, _error} -> - nil + {:ok, nil} -> + # Role doesn't exist - create it + Mv.Authorization.Role + |> Ash.Changeset.for_create(:create_role_with_system_flag, role_data) + |> Ash.create!(authorize?: false, domain: Mv.Authorization) + + {:error, error} -> + IO.puts("Warning: Failed to check for role #{role_data.name}: #{inspect(error)}") + end +end) + +# Get admin role for assignment to admin user +admin_role = + case Mv.Authorization.Role + |> Ash.Query.filter(name == "Admin") + |> Ash.read_one(authorize?: false, domain: Mv.Authorization) do + {:ok, role} when not is_nil(role) -> role + _ -> nil end if is_nil(admin_role) do diff --git a/priv/resource_snapshots/repo/members/20260125155125.json b/priv/resource_snapshots/repo/members/20260125155125.json new file mode 100644 index 0000000..3af9f69 --- /dev/null +++ b/priv/resource_snapshots/repo/members/20260125155125.json @@ -0,0 +1,221 @@ +{ + "attributes": [ + { + "allow_nil?": false, + "default": "fragment(\"uuid_generate_v7()\")", + "generated?": false, + "precision": null, + "primary_key?": true, + "references": null, + "scale": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "first_name", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "last_name", + "type": "text" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "email", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "join_date", + "type": "date" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "exit_date", + "type": "date" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "notes", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "city", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "street", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "house_number", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "postal_code", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "search_vector", + "type": "tsvector" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "membership_fee_start_date", + "type": "date" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": { + "deferrable": false, + "destination_attribute": "id", + "destination_attribute_default": null, + "destination_attribute_generated": null, + "index?": false, + "match_type": null, + "match_with": null, + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "name": "members_membership_fee_type_id_fkey", + "on_delete": null, + "on_update": null, + "primary_key?": true, + "schema": "public", + "table": "membership_fee_types" + }, + "scale": null, + "size": null, + "source": "membership_fee_type_id", + "type": "uuid" + } + ], + "base_filter": null, + "check_constraints": [], + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "107B69E0A6FDBE7FAE4B1EABBF3E8C3B1F004B8D96B3759C95071169288968CC", + "identities": [ + { + "all_tenants?": false, + "base_filter": null, + "index_name": "members_unique_email_index", + "keys": [ + { + "type": "atom", + "value": "email" + } + ], + "name": "unique_email", + "nils_distinct?": true, + "where": null + } + ], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.Mv.Repo", + "schema": null, + "table": "members" +} \ No newline at end of file diff --git a/priv/resource_snapshots/repo/users/20260125155125.json b/priv/resource_snapshots/repo/users/20260125155125.json new file mode 100644 index 0000000..c214ad5 --- /dev/null +++ b/priv/resource_snapshots/repo/users/20260125155125.json @@ -0,0 +1,172 @@ +{ + "attributes": [ + { + "allow_nil?": false, + "default": "fragment(\"gen_random_uuid()\")", + "generated?": false, + "precision": null, + "primary_key?": true, + "references": null, + "scale": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "email", + "type": "citext" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "hashed_password", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "oidc_id", + "type": "text" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": { + "deferrable": false, + "destination_attribute": "id", + "destination_attribute_default": null, + "destination_attribute_generated": null, + "index?": false, + "match_type": null, + "match_with": null, + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "name": "users_role_id_fkey", + "on_delete": "restrict", + "on_update": null, + "primary_key?": true, + "schema": "public", + "table": "roles" + }, + "scale": null, + "size": null, + "source": "role_id", + "type": "uuid" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": { + "deferrable": false, + "destination_attribute": "id", + "destination_attribute_default": null, + "destination_attribute_generated": null, + "index?": false, + "match_type": null, + "match_with": null, + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "name": "users_member_id_fkey", + "on_delete": "nilify", + "on_update": null, + "primary_key?": true, + "schema": "public", + "table": "members" + }, + "scale": null, + "size": null, + "source": "member_id", + "type": "uuid" + } + ], + "base_filter": null, + "check_constraints": [], + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "3E8D3C1A8834053B947F08369B81216A0B13019E5FD6FBFB706968FABA49EC06", + "identities": [ + { + "all_tenants?": false, + "base_filter": null, + "index_name": "users_unique_email_index", + "keys": [ + { + "type": "atom", + "value": "email" + } + ], + "name": "unique_email", + "nils_distinct?": true, + "where": null + }, + { + "all_tenants?": false, + "base_filter": null, + "index_name": "users_unique_member_index", + "keys": [ + { + "type": "atom", + "value": "member_id" + } + ], + "name": "unique_member", + "nils_distinct?": true, + "where": null + }, + { + "all_tenants?": false, + "base_filter": null, + "index_name": "users_unique_oidc_id_index", + "keys": [ + { + "type": "atom", + "value": "oidc_id" + } + ], + "name": "unique_oidc_id", + "nils_distinct?": true, + "where": null + } + ], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.Mv.Repo", + "schema": null, + "table": "users" +} \ No newline at end of file diff --git a/test/mv/accounts/user_policies_test.exs b/test/mv/accounts/user_policies_test.exs index e04213a..7676403 100644 --- a/test/mv/accounts/user_policies_test.exs +++ b/test/mv/accounts/user_policies_test.exs @@ -354,9 +354,14 @@ defmodule Mv.Accounts.UserPoliciesTest do }) |> Ash.Changeset.set_context(%{private: %{ash_authentication?: true}}) - {:ok, user} = Ash.create(changeset) + {:ok, user} = Ash.create(changeset, domain: Mv.Accounts) assert user.email + + # Verify that default "Mitglied" role was assigned + {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) + assert user_with_role.role != nil + assert user_with_role.role.name == "Mitglied" end test "register_with_rauthy works without actor via AshAuthentication bypass" do diff --git a/test/mv/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs index 77596f6..af28443 100644 --- a/test/mv/helpers/system_actor_test.exs +++ b/test/mv/helpers/system_actor_test.exs @@ -265,16 +265,10 @@ defmodule Mv.Helpers.SystemActorTest do end describe "edge cases" do - test "raises error if admin user has no role", %{admin_user: admin_user} do - system_actor = SystemActor.get_system_actor() - - # Remove role from admin user - admin_user - |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, nil, type: :append_and_remove) - |> Ash.update!(actor: system_actor) - - # Delete system user to force fallback + test "raises error if admin user has invalid role (role loading fails)", %{ + admin_user: admin_user + } do + # Delete system user to force fallback to admin user system_actor = SystemActor.get_system_actor() case Accounts.User @@ -287,12 +281,29 @@ defmodule Mv.Helpers.SystemActorTest do :ok end - SystemActor.invalidate_cache() + # Test that NOT NULL + FK constraints prevent setting role_id to NULL + # We verify this by attempting to set role_id to NULL and expecting a constraint violation + admin_user_id = Ecto.UUID.cast!(admin_user.id) + admin_user_id_binary = Ecto.UUID.dump!(admin_user_id) - # Should raise error because admin user has no role - assert_raise RuntimeError, ~r/System actor must have a role assigned/, fn -> - SystemActor.get_system_actor() - end + # Attempting to set role_id to NULL should fail due to NOT NULL constraint + assert_raise Postgrex.Error, + ~r/null value in column.*role_id.*violates not-null constraint/i, + fn -> + Ecto.Adapters.SQL.query!( + Mv.Repo, + """ + UPDATE users + SET role_id = NULL + WHERE id = $1::uuid + """, + [admin_user_id_binary] + ) + end + + # Note: With NOT NULL + FK constraints, we can't test the "no role" case directly + # because the database prevents it. This is the desired behavior - the constraints + # guarantee that role_id is always valid. end test "handles concurrent calls without race conditions" do @@ -368,21 +379,32 @@ defmodule Mv.Helpers.SystemActorTest do end end - test "raises error if system user has no role", %{system_user: system_user} do - system_actor = SystemActor.get_system_actor() + test "raises error if system user has invalid role (role loading fails)", %{ + system_user: system_user + } do + # Test that NOT NULL + FK constraints prevent setting role_id to NULL + # We verify this by attempting to set role_id to NULL and expecting a constraint violation + system_user_id = Ecto.UUID.cast!(system_user.id) + system_user_id_binary = Ecto.UUID.dump!(system_user_id) - # Remove role from system user - system_user - |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, nil, type: :append_and_remove) - |> Ash.update!(actor: system_actor) + # Attempting to set role_id to NULL should fail due to NOT NULL constraint + assert_raise Postgrex.Error, + ~r/null value in column.*role_id.*violates not-null constraint/i, + fn -> + Ecto.Adapters.SQL.query!( + Mv.Repo, + """ + UPDATE users + SET role_id = NULL + WHERE id = $1::uuid + """, + [system_user_id_binary] + ) + end - SystemActor.invalidate_cache() - - # Should raise error because system user has no role - assert_raise RuntimeError, ~r/System actor must have a role assigned/, fn -> - SystemActor.get_system_actor() - end + # Note: With NOT NULL + FK constraints, we can't test the "no role" case directly + # because the database prevents it. This is the desired behavior - the constraints + # guarantee that role_id is always valid. end end end diff --git a/test/seeds_test.exs b/test/seeds_test.exs index 3472616..67b376e 100644 --- a/test/seeds_test.exs +++ b/test/seeds_test.exs @@ -121,4 +121,140 @@ defmodule Mv.SeedsTest do assert :suspended in all_cycle_statuses, "At least one cycle should be suspended" end end + + describe "Authorization roles (from seeds)" do + test "creates all 5 authorization roles with correct permission sets" do + # Run seeds once for this test + Code.eval_file("priv/repo/seeds.exs") + {:ok, roles} = Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) + + assert length(roles) >= 5, "Should have at least 5 roles" + + # Check each role + role_configs = [ + {"Mitglied", "own_data", true}, + {"Vorstand", "read_only", false}, + {"Kassenwart", "normal_user", false}, + {"Buchhaltung", "read_only", false}, + {"Admin", "admin", false} + ] + + Enum.each(role_configs, fn {name, perm_set, is_system} -> + role = Enum.find(roles, &(&1.name == name)) + assert role, "Role #{name} should exist" + assert role.permission_set_name == perm_set + assert role.is_system_role == is_system + end) + end + + test "Mitglied role is marked as system role" do + Code.eval_file("priv/repo/seeds.exs") + + {:ok, mitglied} = + Mv.Authorization.Role + |> Ash.Query.filter(name == "Mitglied") + |> Ash.read_one(domain: Mv.Authorization, authorize?: false) + + assert mitglied.is_system_role == true + end + + test "all roles have valid permission_set_names" do + Code.eval_file("priv/repo/seeds.exs") + + {:ok, roles} = Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) + + valid_sets = + Mv.Authorization.PermissionSets.all_permission_sets() + |> Enum.map(&Atom.to_string/1) + + Enum.each(roles, fn role -> + assert role.permission_set_name in valid_sets, + "Role #{role.name} has invalid permission_set_name: #{role.permission_set_name}" + end) + end + + test "assigns Admin role to ADMIN_EMAIL user" do + Code.eval_file("priv/repo/seeds.exs") + + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" + + {:ok, admin_user} = + Mv.Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) + + assert admin_user != nil, "Admin user should exist after seeds run" + + {:ok, admin_user_with_role} = + Ash.load(admin_user, :role, domain: Mv.Accounts, authorize?: false) + + assert admin_user_with_role.role != nil, "Admin user should have a role assigned" + assert admin_user_with_role.role.name == "Admin" + assert admin_user_with_role.role.permission_set_name == "admin" + end + end + + describe "Authorization role assignment" do + test "does not change role of users who already have a role" do + # Seeds once (creates Admin with Admin role) + Code.eval_file("priv/repo/seeds.exs") + + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" + + {:ok, admin_user} = + Mv.Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) + + assert admin_user != nil, "Admin user should exist after seeds run" + + {:ok, admin_user_with_role} = + Ash.load(admin_user, :role, domain: Mv.Accounts, authorize?: false) + + assert admin_user_with_role.role != nil, "Admin user should have a role assigned" + original_role_id = admin_user_with_role.role_id + assert admin_user_with_role.role.name == "Admin" + + # Seeds again + Code.eval_file("priv/repo/seeds.exs") + + # Admin reloaded + {:ok, admin_reloaded} = + Mv.Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) + + assert admin_reloaded != nil, "Admin user should still exist after re-running seeds" + + {:ok, admin_reloaded_with_role} = + Ash.load(admin_reloaded, :role, domain: Mv.Accounts, authorize?: false) + + assert admin_reloaded_with_role.role != nil, + "Admin user should still have a role after re-running seeds" + + assert admin_reloaded_with_role.role_id == original_role_id + assert admin_reloaded_with_role.role.name == "Admin" + end + + test "role creation is idempotent" do + Code.eval_file("priv/repo/seeds.exs") + + {:ok, roles_1} = + Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) + + Code.eval_file("priv/repo/seeds.exs") + + {:ok, roles_2} = + Ash.read(Mv.Authorization.Role, domain: Mv.Authorization, authorize?: false) + + assert length(roles_1) == length(roles_2), + "Role count should remain same after re-running seeds" + + # Each role should appear exactly once + role_names = Enum.map(roles_2, & &1.name) + + assert length(role_names) == length(Enum.uniq(role_names)), + "Each role name should appear exactly once" + end + end end diff --git a/test/support/data_case.ex b/test/support/data_case.ex index 4ba75ef..630125c 100644 --- a/test/support/data_case.ex +++ b/test/support/data_case.ex @@ -16,6 +16,8 @@ defmodule Mv.DataCase do use ExUnit.CaseTemplate + require Ash.Query + using do quote do alias Mv.Repo @@ -29,6 +31,10 @@ defmodule Mv.DataCase do setup tags do Mv.DataCase.setup_sandbox(tags) + # Ensure "Mitglied" role exists for default role assignment to work in tests + # Note: This runs in every test because each test runs in a sandboxed database. + # The check is fast (single query) and idempotent (skips if role exists). + Mv.DataCase.ensure_default_role() :ok end @@ -42,6 +48,36 @@ defmodule Mv.DataCase do pid end + @doc """ + Ensures the default "Mitglied" role exists in the test database. + + This is necessary because the role_id attribute's default function expects this role to exist. + Tests run in sandbox mode, so the role needs to be created for each test. + """ + def ensure_default_role do + # Check if "Mitglied" role already exists + case Mv.Authorization.Role.get_mitglied_role() do + {:ok, nil} -> + # Create the role if it doesn't exist + Mv.Authorization.Role + |> Ash.Changeset.for_create(:create_role_with_system_flag, %{ + name: "Mitglied", + description: "Default member role with access to own data only", + permission_set_name: "own_data", + is_system_role: true + }) + |> Ash.create!(authorize?: false, domain: Mv.Authorization) + + {:ok, _role} -> + # Role already exists, do nothing + :ok + + {:error, _error} -> + # Ignore errors (e.g., in tests that don't need roles) + :ok + end + end + @doc """ A helper that transforms changeset errors into a map of messages. diff --git a/test/support/fixtures.ex b/test/support/fixtures.ex index 29726ef..23d4aa7 100644 --- a/test/support/fixtures.ex +++ b/test/support/fixtures.ex @@ -182,6 +182,56 @@ defmodule Mv.Fixtures do user_with_role end + @doc """ + Creates a user with password authentication and a specific role. + + This is useful for tests that need to use password-based registration + but also require the user to have a role for authorization. + + ## Parameters + - `email` - User email (defaults to unique generated email) + - `password` - User password (defaults to "testpassword123") + - `permission_set_name` - The permission set name (defaults to "own_data") + + ## Returns + - User struct with role preloaded + + ## Examples + + iex> user = password_user_with_role_fixture() + iex> user.role.permission_set_name + "own_data" + + """ + def password_user_with_role_fixture(opts \\ %{}) do + email = Map.get(opts, :email, "user#{System.unique_integer([:positive])}@example.com") + password = Map.get(opts, :password, "testpassword123") + permission_set_name = Map.get(opts, :permission_set_name, "own_data") + + # Create role with permission set + role = role_fixture(permission_set_name) + + # Create user with password (without password_confirmation as it's optional) + {:ok, user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{ + email: email, + password: password + }) + |> Ash.create() + + # Assign role to user + {:ok, user} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove) + |> Ash.update() + + # Reload user with role preloaded + {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts) + user_with_role + end + @doc """ Creates a member with an actor (for use in tests with policies).