diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 592ac00..7ec870b 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -442,6 +442,15 @@ defmodule Mv.Accounts.User do 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. + + ## 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 @@ -454,9 +463,20 @@ defmodule Mv.Accounts.User do """ @spec default_role_id() :: Ecto.UUID.t() | nil def default_role_id do - case Mv.Authorization.Role.get_mitglied_role() do - {:ok, %Mv.Authorization.Role{id: role_id}} -> role_id - _ -> nil + # Cache in process dictionary to avoid repeated queries + 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 + + Process.put({__MODULE__, :default_role_id}, role_id) + role_id + + cached_role_id -> + cached_role_id end 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 ff954e4..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 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/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs index 48e63f7..af28443 100644 --- a/test/mv/helpers/system_actor_test.exs +++ b/test/mv/helpers/system_actor_test.exs @@ -4,8 +4,6 @@ defmodule Mv.Helpers.SystemActorTest do """ use Mv.DataCase, async: false - import Ecto.Query - alias Mv.Helpers.SystemActor alias Mv.Authorization alias Mv.Accounts @@ -267,18 +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 - # Remove role from admin user by directly setting role_id to NULL in database - # (We can't use Ash because allow_nil? false prevents setting role_id to nil) - # Convert UUID to binary format for Postgrex - admin_user_id = Ecto.UUID.cast!(admin_user.id) - - Mv.Repo.update_all( - from(u in "users", where: u.id == type(^admin_user_id, :binary_id)), - set: [role_id: nil] - ) - - # 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 @@ -291,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 @@ -372,23 +379,32 @@ defmodule Mv.Helpers.SystemActorTest do end end - test "raises error if system user has no role", %{system_user: system_user} do - # Remove role from system user by directly setting role_id to NULL in database - # (We can't use Ash because allow_nil? false prevents setting role_id to nil) - # Convert UUID to binary format for Postgrex + 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) - Mv.Repo.update_all( - from(u in "users", where: u.id == type(^system_user_id, :binary_id)), - set: [role_id: nil] - ) + # 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