diff --git a/lib/mv_web/authorization.ex b/lib/mv_web/authorization.ex
index 18ecd70..95a8524 100644
--- a/lib/mv_web/authorization.ex
+++ b/lib/mv_web/authorization.ex
@@ -106,14 +106,18 @@ defmodule MvWeb.Authorization do
iex> can_access_page?(mitglied, "/members")
false
"""
- @spec can_access_page?(map() | nil, String.t()) :: boolean()
+ @spec can_access_page?(map() | nil, String.t() | Phoenix.VerifiedRoutes.unverified_path()) ::
+ boolean()
def can_access_page?(nil, _page_path), do: false
def can_access_page?(user, page_path) do
+ # Convert verified route to string if needed
+ page_path_str = if is_binary(page_path), do: page_path, else: to_string(page_path)
+
with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- user,
{:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name),
permissions <- PermissionSets.get_permissions(ps_atom) do
- page_matches?(permissions.pages, page_path)
+ page_matches?(permissions.pages, page_path_str)
else
_ -> false
end
diff --git a/lib/mv_web/components/layouts/navbar.ex b/lib/mv_web/components/layouts/navbar.ex
index 692f949..e3e9319 100644
--- a/lib/mv_web/components/layouts/navbar.ex
+++ b/lib/mv_web/components/layouts/navbar.ex
@@ -34,9 +34,9 @@ defmodule MvWeb.Layouts.Navbar do
<.link navigate="/settings">{gettext("Global Settings")}
- <%= if can_access_page?(@current_user, "/admin/roles") do %>
+ <%= if can_access_page?(@current_user, ~p"/admin/roles") do %>
- <.link navigate="/admin/roles">{gettext("Roles")}
+ <.link navigate={~p"/admin/roles"}>{gettext("Roles")}
<% end %>
diff --git a/lib/mv_web/live/role_live/form.ex b/lib/mv_web/live/role_live/form.ex
index 6388ec1..7b74c7e 100644
--- a/lib/mv_web/live/role_live/form.ex
+++ b/lib/mv_web/live/role_live/form.ex
@@ -15,7 +15,7 @@ defmodule MvWeb.RoleLive.Form do
alias Mv.Authorization.PermissionSets
- import MvWeb.RoleLive.Helpers
+ import MvWeb.RoleLive.Helpers, only: [format_error: 1]
on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded}
diff --git a/lib/mv_web/live/role_live/helpers.ex b/lib/mv_web/live/role_live/helpers.ex
index 9d4e77d..8fbc544 100644
--- a/lib/mv_web/live/role_live/helpers.ex
+++ b/lib/mv_web/live/role_live/helpers.ex
@@ -6,6 +6,7 @@ defmodule MvWeb.RoleLive.Helpers do
@doc """
Formats an error for display to the user.
+ Extracts error messages from Ash.Error.Invalid and joins them.
"""
@spec format_error(Ash.Error.Invalid.t() | String.t() | any()) :: String.t()
def format_error(%Ash.Error.Invalid{} = error) do
@@ -24,4 +25,20 @@ defmodule MvWeb.RoleLive.Helpers do
def permission_set_badge_class("normal_user"), do: "badge badge-success badge-sm"
def permission_set_badge_class("admin"), do: "badge badge-error badge-sm"
def permission_set_badge_class(_), do: "badge badge-ghost badge-sm"
+
+ @doc """
+ Builds Ash options with actor and domain, ensuring actor is never nil in real paths.
+ """
+ @spec opts_with_actor(keyword(), map() | nil, atom()) :: keyword()
+ def opts_with_actor(base_opts \\ [], actor, domain) do
+ opts = Keyword.put(base_opts, :domain, domain)
+
+ if actor do
+ Keyword.put(opts, :actor, actor)
+ else
+ require Logger
+ Logger.warning("opts_with_actor called with nil actor - this may bypass policies")
+ opts
+ end
+ end
end
diff --git a/lib/mv_web/live/role_live/index.ex b/lib/mv_web/live/role_live/index.ex
index d34cb22..9d75da6 100644
--- a/lib/mv_web/live/role_live/index.ex
+++ b/lib/mv_web/live/role_live/index.ex
@@ -21,7 +21,8 @@ defmodule MvWeb.RoleLive.Index do
require Ash.Query
- import MvWeb.RoleLive.Helpers
+ import MvWeb.RoleLive.Helpers,
+ only: [format_error: 1, permission_set_badge_class: 1, opts_with_actor: 3]
on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded}
@@ -125,33 +126,28 @@ defmodule MvWeb.RoleLive.Index do
end
end
- # Loads all user counts for roles in a single query to avoid N+1 queries
+ # Loads all user counts for roles using DB-side aggregation for better performance
@spec load_user_counts([Mv.Authorization.Role.t()], map() | nil) :: %{
Ecto.UUID.t() => non_neg_integer()
}
- defp load_user_counts(roles, actor) do
+ defp load_user_counts(roles, _actor) do
role_ids = Enum.map(roles, & &1.id)
- # Load all users with role_id in a single query
- opts = [domain: Mv.Accounts]
- opts = if actor, do: Keyword.put(opts, :actor, actor), else: opts
+ # Use Ecto directly for efficient GROUP BY COUNT query
+ # This is much more performant than loading all users and counting in Elixir
+ # Note: We bypass Ash here for performance, but this is a simple read-only query
+ import Ecto.Query
- users =
- case Ash.read(
- Accounts.User
- |> Ash.Query.filter(role_id in ^role_ids)
- |> Ash.Query.select([:role_id]),
- opts
- ) do
- {:ok, users_list} -> users_list
- {:error, _} -> []
- end
+ query =
+ from u in Accounts.User,
+ where: u.role_id in ^role_ids,
+ group_by: u.role_id,
+ select: {u.role_id, count(u.id)}
- # Group by role_id and count
- users
- |> Enum.group_by(& &1.role_id)
- |> Enum.map(fn {role_id, users_list} -> {role_id, length(users_list)} end)
- |> Map.new()
+ results = Mv.Repo.all(query)
+
+ results
+ |> Enum.into(%{}, fn {role_id, count} -> {role_id, count} end)
end
# Gets user count from preloaded assigns map
@@ -164,8 +160,7 @@ defmodule MvWeb.RoleLive.Index do
# Recalculates user count for a specific role (used before deletion)
@spec recalculate_user_count(Mv.Authorization.Role.t(), map() | nil) :: non_neg_integer()
defp recalculate_user_count(role, actor) do
- opts = [domain: Mv.Accounts]
- opts = if actor, do: Keyword.put(opts, :actor, actor), else: opts
+ opts = opts_with_actor([], actor, Mv.Accounts)
case Ash.count(Accounts.User |> Ash.Query.filter(role_id == ^role.id), opts) do
{:ok, count} -> count
diff --git a/lib/mv_web/live/role_live/show.ex b/lib/mv_web/live/role_live/show.ex
index 3f15155..7184b68 100644
--- a/lib/mv_web/live/role_live/show.ex
+++ b/lib/mv_web/live/role_live/show.ex
@@ -16,7 +16,8 @@ defmodule MvWeb.RoleLive.Show do
require Ash.Query
- import MvWeb.RoleLive.Helpers
+ import MvWeb.RoleLive.Helpers,
+ only: [format_error: 1, permission_set_badge_class: 1, opts_with_actor: 3]
on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded}
@@ -143,8 +144,7 @@ defmodule MvWeb.RoleLive.Show do
# Recalculates user count for a specific role (used before deletion)
@spec recalculate_user_count(Mv.Authorization.Role.t(), map() | nil) :: non_neg_integer()
defp recalculate_user_count(role, actor) do
- opts = [domain: Mv.Accounts]
- opts = if actor, do: Keyword.put(opts, :actor, actor), else: opts
+ opts = opts_with_actor([], actor, Mv.Accounts)
case Ash.count(Accounts.User |> Ash.Query.filter(role_id == ^role.id), opts) do
{:ok, count} -> count
diff --git a/lib/mv_web/live_helpers.ex b/lib/mv_web/live_helpers.ex
index 1835cba..f217ee2 100644
--- a/lib/mv_web/live_helpers.ex
+++ b/lib/mv_web/live_helpers.ex
@@ -49,8 +49,14 @@ defmodule MvWeb.LiveHelpers do
opts = [domain: Mv.Accounts, actor: user]
case Ash.load(user, :role, opts) do
- {:ok, loaded_user} -> loaded_user
- {:error, _} -> user
+ {:ok, loaded_user} ->
+ loaded_user
+
+ {:error, error} ->
+ # Log warning if role loading fails - this can cause authorization issues
+ require Logger
+ Logger.warning("Failed to load role for user #{user.id}: #{inspect(error)}")
+ user
end
end
end
diff --git a/lib/mv_web/router.ex b/lib/mv_web/router.ex
index e73c926..682b672 100644
--- a/lib/mv_web/router.ex
+++ b/lib/mv_web/router.ex
@@ -46,7 +46,10 @@ defmodule MvWeb.Router do
AshAuthentication-specific: We define that all routes can only be accessed when the user is signed in.
"""
ash_authentication_live_session :authentication_required,
- on_mount: {MvWeb.LiveUserAuth, :live_user_required} do
+ on_mount: [
+ {MvWeb.LiveUserAuth, :live_user_required},
+ {MvWeb.LiveHelpers, :ensure_user_role_loaded}
+ ] do
live "/", MemberLive.Index, :index
live "/members", MemberLive.Index, :index