From 9f66708f79e074962249f313460628f16e5c4aa0 Mon Sep 17 00:00:00 2001 From: JuanEstebanPradaEspinosa Date: Mon, 11 May 2026 02:24:12 +0200 Subject: [PATCH 1/4] feat(permissions): Added member roles & permissions Plug pipline authorize added for every request check permissions by controller --- lib/mailgun_logger/roles/roles.ex | 34 +++- lib/mailgun_logger/seeder.ex | 3 +- lib/mailgun_logger/users/user.ex | 17 +- lib/mailgun_logger/users/users.ex | 12 +- lib/mailgun_logger_web.ex | 3 + .../controllers/event_controller.ex | 11 ++ .../controllers/page_controller.ex | 13 ++ .../controllers/profile_controller.ex | 9 + .../controllers/user_controller.ex | 163 ++++++++++++++++-- lib/mailgun_logger_web/plugs/authorize.ex | 63 +++++++ .../templates/layout/app.html.heex | 88 +++++++--- .../templates/user/edit.html.heex | 2 +- .../templates/user/form.html.heex | 1 + .../templates/user/new.html.heex | 2 +- test/lib/mailgun_logger/users/user_test.exs | 11 ++ test/lib/mailgun_logger/users/users_test.exs | 21 +++ 16 files changed, 407 insertions(+), 46 deletions(-) create mode 100644 lib/mailgun_logger_web/plugs/authorize.ex create mode 100644 test/lib/mailgun_logger/users/users_test.exs diff --git a/lib/mailgun_logger/roles/roles.ex b/lib/mailgun_logger/roles/roles.ex index 8a873bb..44d3cc7 100644 --- a/lib/mailgun_logger/roles/roles.ex +++ b/lib/mailgun_logger/roles/roles.ex @@ -8,11 +8,29 @@ defmodule MailgunLogger.Roles do @superuser_role "superuser" @admin_role "admin" + # Create an new role + @member_role "member" + + # Task 1: + # Permissions for member is looking at the events + details on \events pages && Profile changes on /Profile + # (Not: stats, Accounts, Users) interface & router off limits + # + # Update seeds to add the member role in database to use in the application [] + + # Task 2: + # Admin and superuser roles need to have the permissions to edit user & create user + # can add 1 or multiple roles to a user + # An user except from the superuser (owner or first to login) cannot downgrade himself! + # Write an unit test (Phoenix/ExUnit) for these task and permissions + # + ######################################################### - @default_actions ~w() + @default_actions ~w(view_profile) + + @member_actions ~w(view_event view_stats view_graphs) ++ @default_actions - @admin_actions ~w(do_stuff) ++ @default_actions + @admin_actions ~w(trigger_run view_users edit_user create_user delete_user) ++ @member_actions @superuser_actions ~w() ++ @admin_actions @@ -55,6 +73,14 @@ defmodule MailgunLogger.Roles do Enum.any?(roles, &can?(&1.name, action)) end + # Actions + + # Add compile time permission + for action <- @member_actions do + action = String.to_atom(action) + def can?(@member_role, unquote(action)), do: true + end + for action <- @admin_actions do action = String.to_atom(action) def can?(@admin_role, unquote(action)), do: true @@ -67,14 +93,18 @@ defmodule MailgunLogger.Roles do def can?(_, _), do: false + # Include member in the helper functions def is?(%User{roles: roles}, :superuser), do: is(roles, "superuser") def is?(%User{roles: roles}, :admin), do: is(roles, "admin") + def is?(%User{roles: roles}, :member), do: is(roles, "member") def is?(_, _), do: raise("Roles.is/2 requires roles to be preloaded") defp is(roles, role) when is_binary(role), do: Enum.map(roles, & &1.name) |> Enum.member?(role) def abilities(%User{roles: []}), do: [] def abilities(%User{roles: roles}), do: hd(roles) |> abilities() + + def abilities(%Role{name: "member"}), do: @member_actions def abilities(%Role{name: "admin"}), do: @admin_actions def abilities(%Role{name: "superuser"}), do: @superuser_actions diff --git a/lib/mailgun_logger/seeder.ex b/lib/mailgun_logger/seeder.ex index 6fe0588..2e1aaac 100644 --- a/lib/mailgun_logger/seeder.ex +++ b/lib/mailgun_logger/seeder.ex @@ -6,7 +6,8 @@ defmodule MailgunLogger.Seeder do # alias MailgunLogger.UserRole - @roles [%Role{name: "superuser"}, %Role{name: "admin"}] + # add member role so that you can seed the application with member role! + @roles [%Role{name: "superuser"}, %Role{name: "admin"}, %Role{name: "member"}] def run do Enum.each(@roles, &insert_if_new(&1)) diff --git a/lib/mailgun_logger/users/user.ex b/lib/mailgun_logger/users/user.ex index 9ed75cf..729861f 100644 --- a/lib/mailgun_logger/users/user.ex +++ b/lib/mailgun_logger/users/user.ex @@ -35,6 +35,7 @@ defmodule MailgunLogger.User do field(:reset_token, :string, default: nil) field(:theme, :string, default: "system") field(:password, :string, virtual: true) + field(:role_id, :integer, virtual: true) many_to_many(:roles, Role, join_through: UserRole, on_replace: :delete) @@ -45,7 +46,7 @@ defmodule MailgunLogger.User do @spec changeset(User.t(), map()) :: Ecto.Changeset.t() def changeset(%User{} = user, attrs \\ %{}) do user - |> cast(attrs, [:firstname, :lastname, :email, :password, :theme]) + |> cast(attrs, [:firstname, :lastname, :email, :password, :theme, :role_id]) |> validate_required([:email, :password]) |> update_change(:email, &String.downcase/1) |> validate_format(:email, @email_format) @@ -53,16 +54,18 @@ defmodule MailgunLogger.User do |> unique_constraint(:email) |> hash_password() |> generate_token() + |> put_roles_if_present(attrs) end @doc false @spec update_changeset(User.t(), map()) :: Ecto.Changeset.t() def update_changeset(%User{} = user, attrs \\ %{}) do user - |> cast(attrs, [:firstname, :lastname, :email, :theme]) + |> cast(attrs, [:firstname, :lastname, :email, :theme, :role_id]) |> update_change(:email, &String.downcase/1) |> validate_format(:email, @email_format) |> unique_constraint(:email) + |> put_roles_if_present(attrs) end @doc "Used when creating an admin, e.g. from the setup flow" @@ -109,6 +112,16 @@ defmodule MailgunLogger.User do put_change(changeset, :token, token) end + # Add Roles in the existing alias list with an helper function + defp put_roles_if_present(changeset, %{"role_id" => id}) when id not in [nil, ""] do + case Roles.get_roles_by_id([id]) do + [] -> changeset + roles -> Ecto.Changeset.put_assoc(changeset, :roles, roles) + end + end + + defp put_roles_if_present(changeset, _), do: changeset + @doc false def full_name(nil), do: "" def full_name(%User{lastname: nil, firstname: nil, email: nil}), do: "" diff --git a/lib/mailgun_logger/users/users.ex b/lib/mailgun_logger/users/users.ex index ae6902e..e874d45 100644 --- a/lib/mailgun_logger/users/users.ex +++ b/lib/mailgun_logger/users/users.ex @@ -4,10 +4,20 @@ defmodule MailgunLogger.Users do alias MailgunLogger.Repo alias MailgunLogger.User - @type ecto_user() :: {:ok, User.t()} | {:error, Ecto.Changeset.t()} @type maybe_user() :: User.t() | nil + # Get count of the superusers to keep check that there is at least one superuser + @spec count_superusers() :: non_neg_integer() + def count_superusers do + from(u in User, + join: r in assoc(u, :roles), + where: r.name == "superuser", + select: count(u.id) + ) + |> Repo.one() + end + @spec list_users() :: [User.t()] def list_users() do Repo.all(User) |> Repo.preload(:roles) diff --git a/lib/mailgun_logger_web.ex b/lib/mailgun_logger_web.ex index 1f00c61..58fc44b 100644 --- a/lib/mailgun_logger_web.ex +++ b/lib/mailgun_logger_web.ex @@ -19,12 +19,15 @@ defmodule MailgunLoggerWeb do def static_paths(), do: ~w(assets css fonts images js favicon.ico robots.txt) + # Import the plug in controllers so that plug: :authorize_action can be used def controller do quote do use Phoenix.Controller, formats: [html: "View", json: "View"] import Plug.Conn use Gettext, backend: MailgunLoggerWeb.Gettext alias MailgunLoggerWeb.Router.Helpers, as: Routes + # import the plug in every controller as Plug + alias MailgunLoggerWeb.Plugs.Authorize unquote(verified_routes()) end diff --git a/lib/mailgun_logger_web/controllers/event_controller.ex b/lib/mailgun_logger_web/controllers/event_controller.ex index 5ebb105..baa5928 100644 --- a/lib/mailgun_logger_web/controllers/event_controller.ex +++ b/lib/mailgun_logger_web/controllers/event_controller.ex @@ -4,6 +4,17 @@ defmodule MailgunLoggerWeb.EventController do alias MailgunLogger.Events alias MailgunLogger.Accounts + # Set permissions for this controller + plug Authorize + + @action_permissions %{ + index: :view_event, + show: :view_event, + stored_message: :view_event + } + + def action_permissions, do: @action_permissions + def index(conn, params) do accounts = Accounts.list_accounts() diff --git a/lib/mailgun_logger_web/controllers/page_controller.ex b/lib/mailgun_logger_web/controllers/page_controller.ex index ebe8c35..3cf5163 100644 --- a/lib/mailgun_logger_web/controllers/page_controller.ex +++ b/lib/mailgun_logger_web/controllers/page_controller.ex @@ -1,6 +1,19 @@ defmodule MailgunLoggerWeb.PageController do use MailgunLoggerWeb, :controller + plug Authorize + + @action_permissions %{ + index: :view_event, + stats: :view_stats, + graphs: :view_graphs, + # admin only + trigger_run: :trigger_run, + non_affiliation: :view_event + } + + def action_permissions, do: @action_permissions + alias MailgunLogger.Events alias MailgunLogger.Accounts diff --git a/lib/mailgun_logger_web/controllers/profile_controller.ex b/lib/mailgun_logger_web/controllers/profile_controller.ex index e2c5f4d..57b75a5 100644 --- a/lib/mailgun_logger_web/controllers/profile_controller.ex +++ b/lib/mailgun_logger_web/controllers/profile_controller.ex @@ -1,6 +1,15 @@ defmodule MailgunLoggerWeb.ProfileController do use MailgunLoggerWeb, :controller + plug Authorize + + @action_permissions %{ + edit: :view_profile, + update: :view_profile + } + + def action_permissions, do: @action_permissions + alias MailgunLogger.Users alias MailgunLogger.User diff --git a/lib/mailgun_logger_web/controllers/user_controller.ex b/lib/mailgun_logger_web/controllers/user_controller.ex index 1128c69..8d5ddbf 100644 --- a/lib/mailgun_logger_web/controllers/user_controller.ex +++ b/lib/mailgun_logger_web/controllers/user_controller.ex @@ -3,6 +3,20 @@ defmodule MailgunLoggerWeb.UserController do alias MailgunLogger.Users alias MailgunLogger.User + alias MailgunLogger.Roles + + plug Authorize + + @action_permissions %{ + index: :view_users, + new: :create_user, + create: :create_user, + edit: :edit_user, + update: :edit_user, + delete: :delete_user + } + + def action_permissions, do: @action_permissions def index(conn, _) do users = Users.list_users() @@ -11,42 +25,157 @@ defmodule MailgunLoggerWeb.UserController do def new(conn, _) do changeset = User.changeset(%User{}) - render(conn, :new, changeset: changeset) + render(conn, :new, changeset: changeset, roles: role_options()) end def create(conn, %{"user" => params}) do + params = sanitize_role_params(conn.assigns.current_user, params) + case Users.create_user(params) do - {:ok, _} -> redirect(conn, to: Routes.user_path(conn, :index)) - {:error, changeset} -> render(conn, :new, changeset: changeset) + {:ok, _} -> + conn + |> put_flash(:info, "User created.") + |> redirect(to: Routes.user_path(conn, :index)) + + {:error, changeset} -> + render(conn, :new, changeset: changeset, roles: role_options()) end end def edit(conn, %{"id" => id}) do user = Users.get_user!(id) - changeset = User.changeset(user) - render(conn, :edit, changeset: changeset, user: user) + + if can_manage?(conn.assigns.current_user, user) do + current_role_id = + case user.roles do + [%{id: id} | _] -> id + _ -> nil + end + + changeset = + user + |> User.changeset() + |> Ecto.Changeset.put_change(:role_id, current_role_id) + + render(conn, :edit, changeset: changeset, user: user, roles: role_options()) + else + conn + |> put_flash(:error, "You cannot edit this user.") + |> redirect(to: Routes.user_path(conn, :index)) + end end def update(conn, %{"id" => id, "user" => params}) do user = Users.get_user!(id) + actor = conn.assigns.current_user - case Users.update_user(user, params) do - {:ok, _} -> - redirect(conn, to: Routes.user_path(conn, :index)) + cond do + not can_manage?(actor, user) -> + conn + |> put_flash(:error, "You cannot edit this user.") + |> redirect(to: Routes.user_path(conn, :index)) - {:error, changeset} -> - render(conn, :edit, changeset: changeset, user: user) + changing_own_role?(actor, user, params) -> + conn + |> put_flash(:error, "You cannot change your own role.") + |> redirect(to: Routes.user_path(conn, :edit, user)) + + demoting_last_superuser?(user, params) -> + conn + |> put_flash(:error, "Cannot remove the last superuser.") + |> redirect(to: Routes.user_path(conn, :edit, user)) + + true -> + params = sanitize_role_params(actor, params) + + case Users.update_user(user, params) do + {:ok, _} -> + conn + |> put_flash(:info, "User updated.") + |> redirect(to: Routes.user_path(conn, :index)) + + {:error, changeset} -> + render(conn, :edit, changeset: changeset, user: user, roles: role_options()) + end end end def delete(conn, %{"id" => id}) do - {:ok, _} = - id - |> Users.get_user!() - |> Users.delete_user() + user = Users.get_user!(id) + actor = conn.assigns.current_user + + cond do + actor.id == user.id -> + conn + |> put_flash(:error, "You cannot delete your own account.") + |> redirect(to: Routes.user_path(conn, :index)) + + not can_manage?(actor, user) -> + conn + |> put_flash(:error, "You cannot delete this user.") + |> redirect(to: Routes.user_path(conn, :index)) - conn - |> put_flash(:info, "user deleted successfully.") - |> redirect(to: Routes.user_path(conn, :index)) + Roles.is?(user, :superuser) and Users.count_superusers() <= 1 -> + conn + |> put_flash(:error, "Cannot delete the last superuser.") + |> redirect(to: Routes.user_path(conn, :index)) + + true -> + {:ok, _} = Users.delete_user(user) + + conn + |> put_flash(:info, "User deleted successfully.") + |> redirect(to: Routes.user_path(conn, :index)) + end + end + + # Roles as `{label, value}` tuples for the role dropdown. + defp role_options do + Roles.list_roles() |> Enum.map(&{&1.name, &1.id}) + end + + # Only superusers can manage other superusers. Admins can manage admins/members. + defp can_manage?(actor, target) do + Roles.is?(actor, :superuser) or not Roles.is?(target, :superuser) + end + + # Admins cannot grant the `superuser` role; drop it from incoming params. + defp sanitize_role_params(actor, %{"role_id" => id} = params) when id not in [nil, ""] do + if Roles.is?(actor, :superuser) do + params + else + superuser_id = to_string(Roles.get_role_by_name("superuser").id) + + if to_string(id) == superuser_id do + Map.delete(params, "role_id") + else + params + end + end + end + + defp sanitize_role_params(_actor, params), do: params + + defp changing_own_role?(actor, target, %{"role_id" => new_id}) + when new_id not in [nil, ""] do + if actor.id == target.id do + current_ids = Enum.map(target.roles, &to_string(&1.id)) + to_string(new_id) not in current_ids + else + false + end end + + defp changing_own_role?(_actor, _target, _params), do: false + + defp demoting_last_superuser?(target, %{"role_id" => new_id}) + when new_id not in [nil, ""] do + target_is_superuser = Enum.any?(target.roles, &(&1.name == "superuser")) + superuser_id = to_string(Roles.get_role_by_name("superuser").id) + still_superuser = to_string(new_id) == superuser_id + + target_is_superuser and not still_superuser and Users.count_superusers() <= 1 + end + + defp demoting_last_superuser?(_target, _params), do: false end diff --git a/lib/mailgun_logger_web/plugs/authorize.ex b/lib/mailgun_logger_web/plugs/authorize.ex new file mode 100644 index 0000000..e2ab213 --- /dev/null +++ b/lib/mailgun_logger_web/plugs/authorize.ex @@ -0,0 +1,63 @@ +# Controller plug authorization! Just like auth or setup! +# fail-safe if add new action but forget to declare its permissions -> user denied! +defmodule MailgunLoggerWeb.Plugs.Authorize do + # The plug is used for every request and that is why we can use it to check permissions + # -> struct %Plug.Conn{} carry the request + import Plug.Conn + import Phoenix.Controller + + alias MailgunLogger.Roles + alias MailgunLoggerWeb.Router.Helpers, as: Routes + + # Initialize the plug with the given options + def init(opts), do: opts + + # is the plug itself and runs at Request time + def call(conn, _opts) do + # What the action is being called in phoenix controller module + action = action_name(conn) + + # What controller is being called at request time + controller = controller_module(conn) + + # Get the permission of the controller by reading and module attribute at runtime + permissions = + if function_exported?(controller, :action_permissions, 0) do + controller.action_permissions() + else + %{} + end + + # look for the required permission for the action + required = Map.get(permissions || %{}, action) + + # Get the current user role + user = conn.assigns[:current_user] + + cond do + # fail safe if no permission declared -> Deny + is_nil(required) -> + deny(conn) + + # No user + is_nil(user) -> + deny(conn) + + # Check permissions at compile time + Roles.can?(user, required) -> + conn + + # Deny if permission not found + true -> + deny(conn) + end + end + + # Helper to deny the request + defp deny(conn) do + conn + |> put_flash(:error, "you dont have permission to access this resource or page") + |> redirect(to: Routes.event_path(conn, :index)) + |> halt() + end +end diff --git a/lib/mailgun_logger_web/templates/layout/app.html.heex b/lib/mailgun_logger_web/templates/layout/app.html.heex index 6a78307..241346c 100644 --- a/lib/mailgun_logger_web/templates/layout/app.html.heex +++ b/lib/mailgun_logger_web/templates/layout/app.html.heex @@ -1,14 +1,18 @@ - - - - - - Mailgun Logger v<%= Application.spec(:mailgun_logger, :vsn) %> - - + + + + + + Mailgun Logger v{Application.spec(:mailgun_logger, :vsn)} + + - + + diff --git a/lib/mailgun_logger_web/templates/user/edit.html.heex b/lib/mailgun_logger_web/templates/user/edit.html.heex index f9773ed..0294749 100644 --- a/lib/mailgun_logger_web/templates/user/edit.html.heex +++ b/lib/mailgun_logger_web/templates/user/edit.html.heex @@ -1,4 +1,4 @@

Edit user

-<%= render "form.html", action: Routes.user_path(@conn, :update, @user), changeset: @changeset, new?: false, flash: @flash %> +<%= render "form.html", action: Routes.user_path(@conn, :update, @user), changeset: @changeset, new?: false, flash: @flash, roles: @roles %>
<.link href={Routes.user_path(@conn, :delete, @user)} method="delete" data-confirm="Are you sure?" class="btn btn-danger btn-sm">delete diff --git a/lib/mailgun_logger_web/templates/user/form.html.heex b/lib/mailgun_logger_web/templates/user/form.html.heex index dc048c0..105f33e 100644 --- a/lib/mailgun_logger_web/templates/user/form.html.heex +++ b/lib/mailgun_logger_web/templates/user/form.html.heex @@ -17,6 +17,7 @@ label="Theme" options={[{"System", "system"}, {"Light", "light"}, {"Dark", "dark"}]} /> + <.input field={f[:role_id]} type="select" label="Role" options={@roles} />
{submit("submit", class: "btn btn-primary")} diff --git a/lib/mailgun_logger_web/templates/user/new.html.heex b/lib/mailgun_logger_web/templates/user/new.html.heex index 138200d..e162883 100644 --- a/lib/mailgun_logger_web/templates/user/new.html.heex +++ b/lib/mailgun_logger_web/templates/user/new.html.heex @@ -1,2 +1,2 @@

New user

-<%= render "form.html", action: Routes.user_path(@conn, :create), changeset: @changeset, flash: @flash, new?: true %> +<%= render "form.html", action: Routes.user_path(@conn, :create), changeset: @changeset, flash: @flash, new?: true, roles: @roles %> diff --git a/test/lib/mailgun_logger/users/user_test.exs b/test/lib/mailgun_logger/users/user_test.exs index 25a0d42..a853c36 100644 --- a/test/lib/mailgun_logger/users/user_test.exs +++ b/test/lib/mailgun_logger/users/user_test.exs @@ -2,6 +2,7 @@ defmodule MailgunLogger.UserTest do use MailgunLogger.DataCase alias MailgunLogger.User + # alias MailgunLogger.Role @valid_attrs %{ email: "john.doe@acme.com", @@ -29,4 +30,14 @@ defmodule MailgunLogger.UserTest do refute changeset.valid? assert "has already been taken" in errors_on(changeset).email end + + # # Test update role + # test "updates role when role_id changes" do + # user = insert(:user) + # admin_role = Repo.insert!(%Role{name: "admin"}) + + # changeset = User.update_changeset(user, %{"role_id" => admin_role.id}) + # assert changeset.valid? + # assert get_change(changeset, :roles) == [admin_role] + # end end diff --git a/test/lib/mailgun_logger/users/users_test.exs b/test/lib/mailgun_logger/users/users_test.exs new file mode 100644 index 0000000..09cf6aa --- /dev/null +++ b/test/lib/mailgun_logger/users/users_test.exs @@ -0,0 +1,21 @@ +# defmodule MailgunLogger.Users.UsersTest do +# use MailgunLogger.DataCase + +# alias MailgunLogger.Users + +# describe "count_superusers/0" do +# test "returns 0 when there are no superusers" do +# assert Users.count_superusers() == 0 +# end + +# test "returns the correct count" do +# insert(:superuser) +# insert(:superuser) +# insert(:admin) +# assert Users.count_superusers() == 2 +# end +# end + +# # Create User +# # Update User +# end From 7facb1487adc100d9fe640abbc4c9ef43d22a9ed Mon Sep 17 00:00:00 2001 From: JuanEstebanPradaEspinosa Date: Mon, 11 May 2026 03:00:02 +0200 Subject: [PATCH 2/4] fix: make the role input conditional in form.html.heex --- .../templates/user/form.html.heex | 8 ++++- test/lib/mailgun_logger/users/user_test.exs | 16 ++++----- test/lib/mailgun_logger/users/users_test.exs | 34 +++++++++---------- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/lib/mailgun_logger_web/templates/user/form.html.heex b/lib/mailgun_logger_web/templates/user/form.html.heex index 105f33e..141774a 100644 --- a/lib/mailgun_logger_web/templates/user/form.html.heex +++ b/lib/mailgun_logger_web/templates/user/form.html.heex @@ -17,7 +17,13 @@ label="Theme" options={[{"System", "system"}, {"Light", "light"}, {"Dark", "dark"}]} /> - <.input field={f[:role_id]} type="select" label="Role" options={@roles} /> + <.input + :if={assigns[:roles]} + field={f[:role_id]} + type="select" + label="Role" + options={@roles} + />
{submit("submit", class: "btn btn-primary")} diff --git a/test/lib/mailgun_logger/users/user_test.exs b/test/lib/mailgun_logger/users/user_test.exs index a853c36..7892420 100644 --- a/test/lib/mailgun_logger/users/user_test.exs +++ b/test/lib/mailgun_logger/users/user_test.exs @@ -31,13 +31,13 @@ defmodule MailgunLogger.UserTest do assert "has already been taken" in errors_on(changeset).email end - # # Test update role - # test "updates role when role_id changes" do - # user = insert(:user) - # admin_role = Repo.insert!(%Role{name: "admin"}) + # Test update role + test "updates role when role_id changes" do + user = insert(:user) + admin_role = Repo.insert!(%Role{name: "admin"}) - # changeset = User.update_changeset(user, %{"role_id" => admin_role.id}) - # assert changeset.valid? - # assert get_change(changeset, :roles) == [admin_role] - # end + changeset = User.update_changeset(user, %{"role_id" => admin_role.id}) + assert changeset.valid? + assert get_change(changeset, :roles) == [admin_role] + end end diff --git a/test/lib/mailgun_logger/users/users_test.exs b/test/lib/mailgun_logger/users/users_test.exs index 09cf6aa..a2368e0 100644 --- a/test/lib/mailgun_logger/users/users_test.exs +++ b/test/lib/mailgun_logger/users/users_test.exs @@ -1,21 +1,21 @@ -# defmodule MailgunLogger.Users.UsersTest do -# use MailgunLogger.DataCase +defmodule MailgunLogger.Users.UsersTest do + use MailgunLogger.DataCase -# alias MailgunLogger.Users + alias MailgunLogger.Users -# describe "count_superusers/0" do -# test "returns 0 when there are no superusers" do -# assert Users.count_superusers() == 0 -# end + describe "count_superusers/0" do + test "returns 0 when there are no superusers" do + assert Users.count_superusers() == 0 + end -# test "returns the correct count" do -# insert(:superuser) -# insert(:superuser) -# insert(:admin) -# assert Users.count_superusers() == 2 -# end -# end + test "returns the correct count" do + insert(:superuser) + insert(:superuser) + insert(:admin) + assert Users.count_superusers() == 2 + end + end -# # Create User -# # Update User -# end + # Create User + # Update User +end From 45e7549616e178aacee9b3c80f64b5d69f7ba5be Mon Sep 17 00:00:00 2001 From: JuanEstebanPradaEspinosa Date: Mon, 11 May 2026 17:15:26 +0200 Subject: [PATCH 3/4] fix(guards-function-included-for-on-the-permissions-level-and-controller-level): added in the authorize.ex helper function as guard permissions to allow more specefiek actions --- lib/mailgun_logger/roles/roles.ex | 27 ++++++++- lib/mailgun_logger/users/user.ex | 20 ++++--- lib/mailgun_logger_web.ex | 1 + .../controllers/account_controller.ex | 13 +++++ .../controllers/user_controller.ex | 56 ++++++++----------- .../templates/user/form.html.heex | 23 +++++--- 6 files changed, 89 insertions(+), 51 deletions(-) diff --git a/lib/mailgun_logger/roles/roles.ex b/lib/mailgun_logger/roles/roles.ex index 44d3cc7..640e848 100644 --- a/lib/mailgun_logger/roles/roles.ex +++ b/lib/mailgun_logger/roles/roles.ex @@ -30,9 +30,15 @@ defmodule MailgunLogger.Roles do @member_actions ~w(view_event view_stats view_graphs) ++ @default_actions - @admin_actions ~w(trigger_run view_users edit_user create_user delete_user) ++ @member_actions + @admin_actions ~w( + trigger_run view_users edit_user create_user + delete_user manage_accounts manage_admins + manage_members grant_admin_role grant_member_role + ) ++ @member_actions - @superuser_actions ~w() ++ @admin_actions + @superuser_actions ~w( + manage_superusers grant_superuser_role + ) ++ @admin_actions ######################################################### @@ -109,4 +115,21 @@ defmodule MailgunLogger.Roles do def abilities(%Role{name: "superuser"}), do: @superuser_actions def roles(%User{roles: roles}), do: Enum.map(roles, & &1.name) + + # Guards - hidden cameras permission levels what you can do with a given role + def can_manage?(actor, target) do + required_permission = + cond do + is?(target, :superuser) -> :manage_superusers + is?(target, :admin) -> :manage_admins + true -> :manage_members + end + + can?(actor, required_permission) + end + + def can_grant_role?(actor, role_name) do + required_permission = :"grant_#{role_name}_role" + can?(actor, required_permission) + end end diff --git a/lib/mailgun_logger/users/user.ex b/lib/mailgun_logger/users/user.ex index 729861f..7f49244 100644 --- a/lib/mailgun_logger/users/user.ex +++ b/lib/mailgun_logger/users/user.ex @@ -35,7 +35,7 @@ defmodule MailgunLogger.User do field(:reset_token, :string, default: nil) field(:theme, :string, default: "system") field(:password, :string, virtual: true) - field(:role_id, :integer, virtual: true) + field(:role_ids, {:array, :integer}, virtual: true, default: []) many_to_many(:roles, Role, join_through: UserRole, on_replace: :delete) @@ -46,7 +46,7 @@ defmodule MailgunLogger.User do @spec changeset(User.t(), map()) :: Ecto.Changeset.t() def changeset(%User{} = user, attrs \\ %{}) do user - |> cast(attrs, [:firstname, :lastname, :email, :password, :theme, :role_id]) + |> cast(attrs, [:firstname, :lastname, :email, :password, :theme, :role_ids]) |> validate_required([:email, :password]) |> update_change(:email, &String.downcase/1) |> validate_format(:email, @email_format) @@ -61,7 +61,7 @@ defmodule MailgunLogger.User do @spec update_changeset(User.t(), map()) :: Ecto.Changeset.t() def update_changeset(%User{} = user, attrs \\ %{}) do user - |> cast(attrs, [:firstname, :lastname, :email, :theme, :role_id]) + |> cast(attrs, [:firstname, :lastname, :email, :theme, :role_ids]) |> update_change(:email, &String.downcase/1) |> validate_format(:email, @email_format) |> unique_constraint(:email) @@ -112,12 +112,14 @@ defmodule MailgunLogger.User do put_change(changeset, :token, token) end - # Add Roles in the existing alias list with an helper function - defp put_roles_if_present(changeset, %{"role_id" => id}) when id not in [nil, ""] do - case Roles.get_roles_by_id([id]) do - [] -> changeset - roles -> Ecto.Changeset.put_assoc(changeset, :roles, roles) - end + # Replace the user's roles when role_ids is present in the params. + defp put_roles_if_present(changeset, %{"role_ids" => ids}) when is_list(ids) do + roles = + ids + |> Enum.reject(&(&1 in [nil, ""])) + |> Roles.get_roles_by_id() + + Ecto.Changeset.put_assoc(changeset, :roles, roles) end defp put_roles_if_present(changeset, _), do: changeset diff --git a/lib/mailgun_logger_web.ex b/lib/mailgun_logger_web.ex index 58fc44b..4d4a0f3 100644 --- a/lib/mailgun_logger_web.ex +++ b/lib/mailgun_logger_web.ex @@ -26,6 +26,7 @@ defmodule MailgunLoggerWeb do import Plug.Conn use Gettext, backend: MailgunLoggerWeb.Gettext alias MailgunLoggerWeb.Router.Helpers, as: Routes + # import the plug in every controller as Plug alias MailgunLoggerWeb.Plugs.Authorize diff --git a/lib/mailgun_logger_web/controllers/account_controller.ex b/lib/mailgun_logger_web/controllers/account_controller.ex index 49114fe..cc6b494 100644 --- a/lib/mailgun_logger_web/controllers/account_controller.ex +++ b/lib/mailgun_logger_web/controllers/account_controller.ex @@ -4,6 +4,19 @@ defmodule MailgunLoggerWeb.AccountController do alias MailgunLogger.Accounts alias MailgunLogger.Account + plug Authorize + + @action_permissions %{ + index: :manage_accounts, + new: :manage_accounts, + create: :manage_accounts, + edit: :manage_accounts, + update: :manage_accounts, + delete: :manage_accounts + } + + def action_permissions, do: @action_permissions + def index(conn, _) do accounts = Accounts.list_accounts() render(conn, :index, accounts: accounts) diff --git a/lib/mailgun_logger_web/controllers/user_controller.ex b/lib/mailgun_logger_web/controllers/user_controller.ex index 8d5ddbf..7f220fc 100644 --- a/lib/mailgun_logger_web/controllers/user_controller.ex +++ b/lib/mailgun_logger_web/controllers/user_controller.ex @@ -29,6 +29,7 @@ defmodule MailgunLoggerWeb.UserController do end def create(conn, %{"user" => params}) do + # Check if the users has the permission to grant specific role params = sanitize_role_params(conn.assigns.current_user, params) case Users.create_user(params) do @@ -44,18 +45,16 @@ defmodule MailgunLoggerWeb.UserController do def edit(conn, %{"id" => id}) do user = Users.get_user!(id) + actor = conn.assigns.current_user - if can_manage?(conn.assigns.current_user, user) do - current_role_id = - case user.roles do - [%{id: id} | _] -> id - _ -> nil - end + # Check if the current user can manage the target user same rank of permissions + if Roles.can_manage?(actor, user) do + current_role_ids = Enum.map(user.roles, & &1.id) changeset = user |> User.changeset() - |> Ecto.Changeset.put_change(:role_id, current_role_id) + |> Ecto.Changeset.put_change(:role_ids, current_role_ids) render(conn, :edit, changeset: changeset, user: user, roles: role_options()) else @@ -70,7 +69,7 @@ defmodule MailgunLoggerWeb.UserController do actor = conn.assigns.current_user cond do - not can_manage?(actor, user) -> + not Roles.can_manage?(actor, user) -> conn |> put_flash(:error, "You cannot edit this user.") |> redirect(to: Routes.user_path(conn, :index)) @@ -110,7 +109,7 @@ defmodule MailgunLoggerWeb.UserController do |> put_flash(:error, "You cannot delete your own account.") |> redirect(to: Routes.user_path(conn, :index)) - not can_manage?(actor, user) -> + not Roles.can_manage?(actor, user) -> conn |> put_flash(:error, "You cannot delete this user.") |> redirect(to: Routes.user_path(conn, :index)) @@ -134,33 +133,25 @@ defmodule MailgunLoggerWeb.UserController do Roles.list_roles() |> Enum.map(&{&1.name, &1.id}) end - # Only superusers can manage other superusers. Admins can manage admins/members. - defp can_manage?(actor, target) do - Roles.is?(actor, :superuser) or not Roles.is?(target, :superuser) - end - - # Admins cannot grant the `superuser` role; drop it from incoming params. - defp sanitize_role_params(actor, %{"role_id" => id} = params) when id not in [nil, ""] do - if Roles.is?(actor, :superuser) do - params - else - superuser_id = to_string(Roles.get_role_by_name("superuser").id) + # Keep only the role IDs the actor is allowed to grant. + defp sanitize_role_params(actor, %{"role_ids" => ids} = params) when is_list(ids) do + allowed_ids = + ids + |> Enum.reject(&(&1 in [nil, ""])) + |> Roles.get_roles_by_id() + |> Enum.filter(&Roles.can_grant_role?(actor, &1.name)) + |> Enum.map(&to_string(&1.id)) - if to_string(id) == superuser_id do - Map.delete(params, "role_id") - else - params - end - end + Map.put(params, "role_ids", allowed_ids) end defp sanitize_role_params(_actor, params), do: params - defp changing_own_role?(actor, target, %{"role_id" => new_id}) - when new_id not in [nil, ""] do + defp changing_own_role?(actor, target, %{"role_ids" => ids}) when is_list(ids) do if actor.id == target.id do - current_ids = Enum.map(target.roles, &to_string(&1.id)) - to_string(new_id) not in current_ids + current = target.roles |> Enum.map(&to_string(&1.id)) |> Enum.sort() + incoming = ids |> Enum.reject(&(&1 in [nil, ""])) |> Enum.sort() + current != incoming else false end @@ -168,11 +159,10 @@ defmodule MailgunLoggerWeb.UserController do defp changing_own_role?(_actor, _target, _params), do: false - defp demoting_last_superuser?(target, %{"role_id" => new_id}) - when new_id not in [nil, ""] do + defp demoting_last_superuser?(target, %{"role_ids" => ids}) when is_list(ids) do target_is_superuser = Enum.any?(target.roles, &(&1.name == "superuser")) superuser_id = to_string(Roles.get_role_by_name("superuser").id) - still_superuser = to_string(new_id) == superuser_id + still_superuser = superuser_id in ids target_is_superuser and not still_superuser and Users.count_superusers() <= 1 end diff --git a/lib/mailgun_logger_web/templates/user/form.html.heex b/lib/mailgun_logger_web/templates/user/form.html.heex index 141774a..6b521dd 100644 --- a/lib/mailgun_logger_web/templates/user/form.html.heex +++ b/lib/mailgun_logger_web/templates/user/form.html.heex @@ -17,13 +17,22 @@ label="Theme" options={[{"System", "system"}, {"Light", "light"}, {"Dark", "dark"}]} /> - <.input - :if={assigns[:roles]} - field={f[:role_id]} - type="select" - label="Role" - options={@roles} - /> + <%= if assigns[:roles] do %> + <% selected = Ecto.Changeset.get_field(@changeset, :role_ids) || [] %> + <% slot_count = min(length(selected) + 1, length(@roles)) %> + + <%= for slot <- 0..(slot_count - 1) do %> + <.input + type="select" + name="user[role_ids][]" + id={"user_role_id_#{slot}"} + label={if slot == 0, do: "Primary role", else: "Additional role"} + value={Enum.at(selected, slot)} + options={@roles} + prompt="Select a role" + /> + <% end %> + <% end %>
{submit("submit", class: "btn btn-primary")} From 56b6236317df35c2fbde13e62b2e2df2ff634b25 Mon Sep 17 00:00:00 2001 From: JuanEstebanPradaEspinosa Date: Mon, 11 May 2026 18:10:40 +0200 Subject: [PATCH 4/4] test(roles,-users): permissions tests for roles and create & edit user with role --- lib/mailgun_logger/roles/roles.ex | 6 +- test/lib/mailgun_logger/roles/roles_test.exs | 117 ++++++++++++++++++ .../users/user_controller_test.exs | 0 test/lib/mailgun_logger/users/user_test.exs | 10 -- test/lib/mailgun_logger/users/users_test.exs | 39 +++++- 5 files changed, 152 insertions(+), 20 deletions(-) create mode 100644 test/lib/mailgun_logger/roles/roles_test.exs create mode 100644 test/lib/mailgun_logger/users/user_controller_test.exs diff --git a/lib/mailgun_logger/roles/roles.ex b/lib/mailgun_logger/roles/roles.ex index 640e848..6eccf4a 100644 --- a/lib/mailgun_logger/roles/roles.ex +++ b/lib/mailgun_logger/roles/roles.ex @@ -14,15 +14,13 @@ defmodule MailgunLogger.Roles do # Task 1: # Permissions for member is looking at the events + details on \events pages && Profile changes on /Profile # (Not: stats, Accounts, Users) interface & router off limits - # - # Update seeds to add the member role in database to use in the application [] + # Update seeds to add the member role in database to use in the application # Task 2: # Admin and superuser roles need to have the permissions to edit user & create user # can add 1 or multiple roles to a user - # An user except from the superuser (owner or first to login) cannot downgrade himself! + # An user cannot downgrade himself! # Write an unit test (Phoenix/ExUnit) for these task and permissions - # ######################################################### diff --git a/test/lib/mailgun_logger/roles/roles_test.exs b/test/lib/mailgun_logger/roles/roles_test.exs new file mode 100644 index 0000000..b62acf1 --- /dev/null +++ b/test/lib/mailgun_logger/roles/roles_test.exs @@ -0,0 +1,117 @@ +defmodule MailgunLogger.Roles.RolesTest do + use MailgunLogger.DataCase + + alias MailgunLogger.Roles + + defp superuser, do: build(:user, roles: [build(:role, name: "superuser")]) + defp admin, do: build(:user, roles: [build(:role, name: "admin")]) + defp member, do: build(:user, roles: [build(:role, name: "member")]) + + describe "can?/2 — gate layer (action-level permissions)" do + test "member: allowed actions" do + m = member() + assert Roles.can?(m, :view_profile) + assert Roles.can?(m, :view_event) + assert Roles.can?(m, :view_stats) + assert Roles.can?(m, :view_graphs) + end + + test "member: forbidden actions (admin/superuser territory)" do + m = member() + refute Roles.can?(m, :view_users) + refute Roles.can?(m, :edit_user) + refute Roles.can?(m, :create_user) + refute Roles.can?(m, :delete_user) + refute Roles.can?(m, :manage_accounts) + refute Roles.can?(m, :manage_superusers) + refute Roles.can?(m, :grant_superuser_role) + end + + test "admin: inherits member actions" do + a = admin() + assert Roles.can?(a, :view_profile) + assert Roles.can?(a, :view_event) + assert Roles.can?(a, :view_stats) + assert Roles.can?(a, :view_graphs) + end + + test "admin: has admin-only actions" do + a = admin() + assert Roles.can?(a, :view_users) + assert Roles.can?(a, :edit_user) + assert Roles.can?(a, :create_user) + assert Roles.can?(a, :delete_user) + assert Roles.can?(a, :manage_accounts) + assert Roles.can?(a, :manage_admins) + assert Roles.can?(a, :manage_members) + assert Roles.can?(a, :grant_admin_role) + assert Roles.can?(a, :grant_member_role) + end + + test "admin: cannot touch superuser-only actions" do + a = admin() + refute Roles.can?(a, :manage_superusers) + refute Roles.can?(a, :grant_superuser_role) + end + + test "superuser: inherits all admin and member actions" do + s = superuser() + assert Roles.can?(s, :view_event) + assert Roles.can?(s, :edit_user) + assert Roles.can?(s, :manage_admins) + assert Roles.can?(s, :grant_admin_role) + end + + test "superuser: has superuser-only actions" do + s = superuser() + assert Roles.can?(s, :manage_superusers) + assert Roles.can?(s, :grant_superuser_role) + end + end + + describe "can_manage?/2 — resource layer (who can edit/delete whom)" do + test "superuser can manage anyone" do + s = superuser() + assert Roles.can_manage?(s, superuser()) + assert Roles.can_manage?(s, admin()) + assert Roles.can_manage?(s, member()) + end + + test "admin can manage admins and members, but NOT superusers" do + a = admin() + refute Roles.can_manage?(a, superuser()) + assert Roles.can_manage?(a, admin()) + assert Roles.can_manage?(a, member()) + end + + test "member cannot manage anyone" do + m = member() + refute Roles.can_manage?(m, superuser()) + refute Roles.can_manage?(m, admin()) + refute Roles.can_manage?(m, member()) + end + end + + describe "can_grant_role?/2 — resource layer (who can assign which role)" do + test "superuser can grant any role" do + s = superuser() + assert Roles.can_grant_role?(s, "superuser") + assert Roles.can_grant_role?(s, "admin") + assert Roles.can_grant_role?(s, "member") + end + + test "admin can grant admin and member, but NOT superuser" do + a = admin() + refute Roles.can_grant_role?(a, "superuser") + assert Roles.can_grant_role?(a, "admin") + assert Roles.can_grant_role?(a, "member") + end + + test "member cannot grant any role" do + m = member() + refute Roles.can_grant_role?(m, "superuser") + refute Roles.can_grant_role?(m, "admin") + refute Roles.can_grant_role?(m, "member") + end + end +end diff --git a/test/lib/mailgun_logger/users/user_controller_test.exs b/test/lib/mailgun_logger/users/user_controller_test.exs new file mode 100644 index 0000000..e69de29 diff --git a/test/lib/mailgun_logger/users/user_test.exs b/test/lib/mailgun_logger/users/user_test.exs index 7892420..114539b 100644 --- a/test/lib/mailgun_logger/users/user_test.exs +++ b/test/lib/mailgun_logger/users/user_test.exs @@ -30,14 +30,4 @@ defmodule MailgunLogger.UserTest do refute changeset.valid? assert "has already been taken" in errors_on(changeset).email end - - # Test update role - test "updates role when role_id changes" do - user = insert(:user) - admin_role = Repo.insert!(%Role{name: "admin"}) - - changeset = User.update_changeset(user, %{"role_id" => admin_role.id}) - assert changeset.valid? - assert get_change(changeset, :roles) == [admin_role] - end end diff --git a/test/lib/mailgun_logger/users/users_test.exs b/test/lib/mailgun_logger/users/users_test.exs index a2368e0..3dea7d5 100644 --- a/test/lib/mailgun_logger/users/users_test.exs +++ b/test/lib/mailgun_logger/users/users_test.exs @@ -1,6 +1,5 @@ defmodule MailgunLogger.Users.UsersTest do use MailgunLogger.DataCase - alias MailgunLogger.Users describe "count_superusers/0" do @@ -9,13 +8,41 @@ defmodule MailgunLogger.Users.UsersTest do end test "returns the correct count" do - insert(:superuser) - insert(:superuser) - insert(:admin) + superuser_role = insert(:role, name: "superuser") + admin_role = insert(:role, name: "admin") + insert(:user, roles: [superuser_role]) + insert(:user, roles: [superuser_role]) + insert(:user, roles: [admin_role]) assert Users.count_superusers() == 2 end end - # Create User - # Update User + describe "create_user/1" do + test "creates user with assigned role" do + admin_role = insert(:role, name: "admin") + + {:ok, user} = + Users.create_user(%{ + "email" => "juan@gmail.com", + "password" => "password123456", + "role_ids" => [admin_role.id] + }) + + user = Repo.preload(user, :roles) + assert Enum.map(user.roles, & &1.name) == ["admin"] + end + end + + describe "update_user/2" do + test "replaces existing roles atomically" do + member_role = insert(:role, name: "member") + admin_role = insert(:role, name: "admin") + user = insert(:user, roles: [member_role]) + + {:ok, updated} = Users.update_user(user, %{"role_ids" => [admin_role.id]}) + + updated = Repo.preload(updated, :roles, force: true) + assert Enum.map(updated.roles, & &1.name) == ["admin"] + end + end end