diff --git a/.gitignore b/.gitignore index a155b1a..285a7e7 100644 --- a/.gitignore +++ b/.gitignore @@ -35,3 +35,5 @@ docker-compose.yml node_modules .serena .playwright-mcp +mailgun_logger_accounts_events.sql +*.pem diff --git a/lib/mailgun_logger/roles/roles.ex b/lib/mailgun_logger/roles/roles.ex index 8a873bb..8467628 100644 --- a/lib/mailgun_logger/roles/roles.ex +++ b/lib/mailgun_logger/roles/roles.ex @@ -7,14 +7,23 @@ defmodule MailgunLogger.Roles do @superuser_role "superuser" @admin_role "admin" + @member_role "member" ######################################################### - @default_actions ~w() + @all_roles [@superuser_role, @admin_role, @member_role] + @assignable_roles Enum.reject(@all_roles, &(&1 == @superuser_role)) - @admin_actions ~w(do_stuff) ++ @default_actions + ######################################################### - @superuser_actions ~w() ++ @admin_actions + @default_actions ~w() + @member_actions ~w( + view_events + view_event_details + edit_profile +) ++ @default_actions + @admin_actions ~w(do_stuff assign_roles) ++ @member_actions + @superuser_actions ~w(manage_admins) ++ @admin_actions ######################################################### @@ -36,6 +45,15 @@ defmodule MailgunLogger.Roles do |> Repo.one() end + @spec get_roles_by_names([String.t()]) :: [Role.t()] + def get_roles_by_names([]), do: [] + + def get_roles_by_names(names) do + Role + |> where([r], r.name in ^names) + |> Repo.all() + end + @spec get_by_user(User.t()) :: [Role.t()] def get_by_user(%User{} = user) do user @@ -55,6 +73,11 @@ defmodule MailgunLogger.Roles do Enum.any?(roles, &can?(&1.name, action)) end + 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 @@ -69,14 +92,37 @@ defmodule MailgunLogger.Roles do 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(%User{roles: roles}), + do: + roles + |> Enum.flat_map(&abilities/1) + |> Enum.uniq() + def abilities(%Role{name: "admin"}), do: @admin_actions def abilities(%Role{name: "superuser"}), do: @superuser_actions + def abilities(%Role{name: "member"}), do: @member_actions def roles(%User{roles: roles}), do: Enum.map(roles, & &1.name) + + @spec can_modify_roles?(User.t(), User.t()) :: boolean() + def can_modify_roles?(user, target) do + user.id != target.id and + Enum.any?([:manage_admins, :assign_roles], fn action -> + can?(user, action) + # and not can?(target, action) + # NOTE: For when we don't want admins to manage superusers or allow same role + # role modification, might be overengineering for now + end) + end + + def assignable_roles() do + @assignable_roles + end end diff --git a/lib/mailgun_logger/users/user.ex b/lib/mailgun_logger/users/user.ex index 9ed75cf..3acafd3 100644 --- a/lib/mailgun_logger/users/user.ex +++ b/lib/mailgun_logger/users/user.ex @@ -44,6 +44,8 @@ defmodule MailgunLogger.User do @doc false @spec changeset(User.t(), map()) :: Ecto.Changeset.t() def changeset(%User{} = user, attrs \\ %{}) do + roles = Map.get(attrs, "roles", []) |> Roles.get_roles_by_names() + user |> cast(attrs, [:firstname, :lastname, :email, :password, :theme]) |> validate_required([:email, :password]) @@ -53,21 +55,25 @@ defmodule MailgunLogger.User do |> unique_constraint(:email) |> hash_password() |> generate_token() + |> put_assoc(:roles, roles) end @doc false @spec update_changeset(User.t(), map()) :: Ecto.Changeset.t() def update_changeset(%User{} = user, attrs \\ %{}) do + roles = Map.get(attrs, "roles", []) |> Roles.get_roles_by_names() + user |> cast(attrs, [:firstname, :lastname, :email, :theme]) |> update_change(:email, &String.downcase/1) |> validate_format(:email, @email_format) + |> put_assoc(:roles, roles) |> unique_constraint(:email) end @doc "Used when creating an admin, e.g. from the setup flow" - @spec admin_changeset(User.t(), map()) :: Ecto.Changeset.t() - def admin_changeset(%User{} = user, attrs) do + @spec superuser_changeset(User.t(), map()) :: Ecto.Changeset.t() + def superuser_changeset(%User{} = user, attrs) do user |> cast(attrs, [:email, :password]) |> validate_required([:email, :password]) @@ -77,7 +83,7 @@ defmodule MailgunLogger.User do |> unique_constraint(:email) |> hash_password() |> generate_token() - |> put_assoc(:roles, [Roles.get_role_by_name("admin")]) + |> put_assoc(:roles, [Roles.get_role_by_name("superuser")]) end @doc false diff --git a/lib/mailgun_logger/users/users.ex b/lib/mailgun_logger/users/users.ex index ae6902e..3822653 100644 --- a/lib/mailgun_logger/users/users.ex +++ b/lib/mailgun_logger/users/users.ex @@ -4,7 +4,6 @@ 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 @@ -140,7 +139,7 @@ defmodule MailgunLogger.Users do @spec create_admin(map()) :: ecto_user() def create_admin(attrs) do %User{} - |> User.admin_changeset(attrs) + |> User.superuser_changeset(attrs) |> Repo.insert() end diff --git a/lib/mailgun_logger_web/controllers/profile_controller.ex b/lib/mailgun_logger_web/controllers/profile_controller.ex index e2c5f4d..a0476ff 100644 --- a/lib/mailgun_logger_web/controllers/profile_controller.ex +++ b/lib/mailgun_logger_web/controllers/profile_controller.ex @@ -1,4 +1,5 @@ defmodule MailgunLoggerWeb.ProfileController do + alias MailgunLogger.Roles use MailgunLoggerWeb, :controller alias MailgunLogger.Users @@ -7,10 +8,11 @@ defmodule MailgunLoggerWeb.ProfileController do def edit(conn, _) do user = conn.assigns.current_user changeset = User.changeset(user) + assignable_roles = Roles.assignable_roles() conn |> put_view(MailgunLoggerWeb.UserView) - |> render(:profile, changeset: changeset, user: user) + |> render(:profile, changeset: changeset, user: user, assignable_roles: assignable_roles) end def update(conn, %{"user" => params}) do diff --git a/lib/mailgun_logger_web/controllers/setup_controller.ex b/lib/mailgun_logger_web/controllers/setup_controller.ex index 0c71a58..2c549e9 100644 --- a/lib/mailgun_logger_web/controllers/setup_controller.ex +++ b/lib/mailgun_logger_web/controllers/setup_controller.ex @@ -15,7 +15,7 @@ defmodule MailgunLoggerWeb.SetupController do end end - def create_root(conn, %{"user" => params}) do + def create_root(conn, %{"user" => params}) do # Only allow create_root if there are no users in the database yet, # otherwise it's possible for any unauthenticated request to this # endpoint to make an admin user. diff --git a/lib/mailgun_logger_web/controllers/user_controller.ex b/lib/mailgun_logger_web/controllers/user_controller.ex index 1128c69..ea42bb2 100644 --- a/lib/mailgun_logger_web/controllers/user_controller.ex +++ b/lib/mailgun_logger_web/controllers/user_controller.ex @@ -1,4 +1,5 @@ defmodule MailgunLoggerWeb.UserController do + alias MailgunLogger.Roles use MailgunLoggerWeb, :controller alias MailgunLogger.Users @@ -11,7 +12,8 @@ defmodule MailgunLoggerWeb.UserController do def new(conn, _) do changeset = User.changeset(%User{}) - render(conn, :new, changeset: changeset) + assignable_roles = Roles.assignable_roles() + render(conn, :new, changeset: changeset, assignable_roles: assignable_roles) end def create(conn, %{"user" => params}) do @@ -24,18 +26,45 @@ defmodule MailgunLoggerWeb.UserController do def edit(conn, %{"id" => id}) do user = Users.get_user!(id) changeset = User.changeset(user) - render(conn, :edit, changeset: changeset, user: user) + + render(conn, :edit, + changeset: changeset, + user: user, + assignable_roles: Roles.assignable_roles(), + editable_roles: Roles.can_modify_roles?(conn.assigns.current_user, user) + ) end def update(conn, %{"id" => id, "user" => params}) do - user = Users.get_user!(id) + target = Users.get_user!(id) + actor = conn.assigns.current_user + params = Map.put(params, "roles", Map.get(params, "roles", [])) + + roles_modified? = + MapSet.new(Enum.map(target.roles, & &1.name)) != + MapSet.new(Map.get(params, "roles")) - case Users.update_user(user, params) do - {:ok, _} -> - redirect(conn, to: Routes.user_path(conn, :index)) + if roles_modified? and not Roles.can_modify_roles?(actor, target) do + conn + |> put_flash(:error, "Not authorized to modify roles") + |> redirect(to: Routes.user_path(conn, :edit, target)) + else + case Users.update_user(target, params) do + {:ok, _} -> + redirect(conn, to: Routes.user_path(conn, :index)) - {:error, changeset} -> - render(conn, :edit, changeset: changeset, user: user) + {:error, changeset} -> + render(conn, :edit, + changeset: changeset, + user: target, + assignable_roles: Roles.assignable_roles(), + editable_roles: + Roles.can_modify_roles?( + actor, + target + ) + ) + end end 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..064ee76 --- /dev/null +++ b/lib/mailgun_logger_web/plugs/authorize.ex @@ -0,0 +1,21 @@ +defmodule MailgunLoggerWeb.Plugs.Authorize do + import Plug.Conn + import Phoenix.Controller + alias MailgunLoggerWeb.Router.Helpers + alias MailgunLogger.Roles + + def init(action), do: action + + def call(conn, action) do + user = conn.assigns.current_user + + if Roles.can?(user, action) do + conn + else + conn + |> put_status(:forbidden) + |> redirect(to: Helpers.event_path(conn, :index)) + |> halt() + end + end +end diff --git a/lib/mailgun_logger_web/router.ex b/lib/mailgun_logger_web/router.ex index 7297982..74fa272 100644 --- a/lib/mailgun_logger_web/router.ex +++ b/lib/mailgun_logger_web/router.ex @@ -23,6 +23,10 @@ defmodule MailgunLoggerWeb.Router do plug(MailgunLoggerWeb.Plugs.Auth) end + pipeline :require_admin do + plug MailgunLoggerWeb.Plugs.Authorize, :do_stuff + end + # Always except in prod if Application.compile_env(:mailgun_logger, :env) == :dev do forward("/sent_emails", Bamboo.SentEmailViewerPlug) @@ -71,14 +75,20 @@ defmodule MailgunLoggerWeb.Router do scope "/", MailgunLoggerWeb do pipe_through([:browser, :auth]) - get("/logout", AuthController, :logout) + resources "/events", EventController, only: [:index, :show] + get "/events/:id/stored_message", EventController, :stored_message + + get "/profile", ProfileController, :edit + put "/profile", ProfileController, :update + end + + scope "/", MailgunLoggerWeb do + pipe_through([:browser, :auth, :require_admin]) + resources("/events", EventController, only: [:index, :show]) - get("/events/:id/stored_message", EventController, :stored_message) resources("/accounts", AccountController, except: [:show]) - get("/profile", ProfileController, :edit) - put("/profile", ProfileController, :update) resources("/users", UserController, except: [:show]) get("/", PageController, :index) diff --git a/lib/mailgun_logger_web/templates/account/edit.html.heex b/lib/mailgun_logger_web/templates/account/edit.html.heex index 18fabb4..2b807d6 100644 --- a/lib/mailgun_logger_web/templates/account/edit.html.heex +++ b/lib/mailgun_logger_web/templates/account/edit.html.heex @@ -1,4 +1,15 @@

Edit account

-<%= render "form.html", action: Routes.account_path(@conn, :update, @account), changeset: @changeset, flash: @flash %> +{render("form.html", + action: Routes.account_path(@conn, :update, @account), + changeset: @changeset, + flash: @flash +)}
-<.link href={Routes.account_path(@conn, :delete, @account)} method="delete" data-confirm="Are you sure?" class="btn btn-danger btn-sm">delete +<.link + href={Routes.account_path(@conn, :delete, @account)} + method="delete" + data-confirm="Are you sure?" + class="btn btn-danger btn-sm" +> + delete + diff --git a/lib/mailgun_logger_web/templates/account/form.html.heex b/lib/mailgun_logger_web/templates/account/form.html.heex index ba990bf..2c214dd 100644 --- a/lib/mailgun_logger_web/templates/account/form.html.heex +++ b/lib/mailgun_logger_web/templates/account/form.html.heex @@ -3,22 +3,32 @@

Oops, something went wrong! Please check the errors below.

-

These settings can be found on your Mailgun dashboard. An account covers a sending domain / API key combo. If you have multiple domains, add them on seperate accounts.

+

+ These settings can be found on your Mailgun dashboard. An account covers a sending domain / API key combo. If you have multiple domains, add them on seperate accounts. +

- <%= Phoenix.Flash.get(@flash, :info) %> + {Phoenix.Flash.get(@flash, :info)}
<.input field={f[:domain]} label="Sending domain" /> -
Find on Mailgun dashboard
+
+ + Find on Mailgun dashboard + +
<.input field={f[:api_key]} label="API key" /> -
Find on Mailgun dashboard
+
+ + Find on Mailgun dashboard + +
<.input field={f[:is_active]} type="checkbox" label="Is active" /> <.input field={f[:is_eu]} type="checkbox" label="Is EU" />
- <%= submit "submit", class: "btn btn-primary"%> + {submit("submit", class: "btn btn-primary")}
diff --git a/lib/mailgun_logger_web/templates/account/index.html.heex b/lib/mailgun_logger_web/templates/account/index.html.heex index 8ee45ba..2fb607f 100644 --- a/lib/mailgun_logger_web/templates/account/index.html.heex +++ b/lib/mailgun_logger_web/templates/account/index.html.heex @@ -12,16 +12,16 @@ <%= for account <- @accounts do %> - - <%= account.domain %> - <%= account.api_key %> - EU - -
active
-
inactive
- - <.link href={Routes.account_path(@conn, :edit, account)}>edit - + + {account.domain} + {account.api_key} + EU + +
active
+
inactive
+ + <.link href={Routes.account_path(@conn, :edit, account)}>edit + <% 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..0d89451 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..99c1226 100644 --- a/lib/mailgun_logger_web/templates/user/edit.html.heex +++ b/lib/mailgun_logger_web/templates/user/edit.html.heex @@ -1,4 +1,19 @@

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, + assignable_roles: @assignable_roles, + user: @user, + editable_roles: @editable_roles +)}
-<.link href={Routes.user_path(@conn, :delete, @user)} method="delete" data-confirm="Are you sure?" class="btn btn-danger btn-sm">delete +<.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..df77603 100644 --- a/lib/mailgun_logger_web/templates/user/form.html.heex +++ b/lib/mailgun_logger_web/templates/user/form.html.heex @@ -2,15 +2,30 @@

Oops, something went wrong! Please check the errors below.

-
{Phoenix.Flash.get(@flash, :info)}
- <.input field={f[:firstname]} label="Firstname" /> <.input field={f[:lastname]} label="Lastname" /> <.input field={f[:email]} label="Email" /> <.input :if={@new?} field={f[:password]} type="password" label="Password" /> + <% assigned_roles = + case assigns do + %{user: %{roles: roles}} -> Enum.map(roles, & &1.name) + _ -> [] + end %> + <%= for role <- @assignable_roles do %> + + <% end %> <.input field={f[:theme]} type="select" diff --git a/lib/mailgun_logger_web/templates/user/index.html.heex b/lib/mailgun_logger_web/templates/user/index.html.heex index fe2570c..b2b09a1 100644 --- a/lib/mailgun_logger_web/templates/user/index.html.heex +++ b/lib/mailgun_logger_web/templates/user/index.html.heex @@ -11,12 +11,12 @@ <%= for user <- @users do %> - - <%= "#{user.firstname || ""} #{user.lastname || ""}" |> String.trim() %> - <%= user.email %> - <%= Enum.map(user.roles, & &1.name) %> - <.link href={Routes.user_path(@conn, :edit, user)}>edit - + + {"#{user.firstname || ""} #{user.lastname || ""}" |> String.trim()} + {user.email} + {user.roles |> Enum.map(& &1.name) |> Enum.join(", ")} + <.link href={Routes.user_path(@conn, :edit, user)}>edit + <% end %> diff --git a/lib/mailgun_logger_web/templates/user/new.html.heex b/lib/mailgun_logger_web/templates/user/new.html.heex index 138200d..5721f2f 100644 --- a/lib/mailgun_logger_web/templates/user/new.html.heex +++ b/lib/mailgun_logger_web/templates/user/new.html.heex @@ -1,2 +1,9 @@

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, + assignable_roles: @assignable_roles, + editable_roles: true +)} diff --git a/lib/mailgun_logger_web/templates/user/profile.html.heex b/lib/mailgun_logger_web/templates/user/profile.html.heex index 08a1e49..ce467d0 100644 --- a/lib/mailgun_logger_web/templates/user/profile.html.heex +++ b/lib/mailgun_logger_web/templates/user/profile.html.heex @@ -1,2 +1,10 @@

Profile

-<%= render "form.html", action: Routes.profile_path(@conn, :update), changeset: @changeset, new?: false, flash: @flash %> +{render("form.html", + action: Routes.profile_path(@conn, :update), + changeset: @changeset, + new?: false, + flash: @flash, + editable_roles: false, + assignable_roles: @assignable_roles, + user: @user +)} diff --git a/priv/repo/migrations/20190606150819_create_roles_users.exs b/priv/repo/migrations/20190606150819_create_roles_users.exs index 1f59067..a6441ec 100644 --- a/priv/repo/migrations/20190606150819_create_roles_users.exs +++ b/priv/repo/migrations/20190606150819_create_roles_users.exs @@ -3,8 +3,8 @@ defmodule MailgunLogger.Repo.Migrations.CreateRolesUsers do def change do create table(:roles_users) do - add(:role_id, references(:roles)) - add(:user_id, references(:users)) + add(:role_id, references(:roles, on_delete: :delete_all), null: false) + add(:user_id, references(:users, on_delete: :delete_all), null: false) timestamps() end 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..878efe2 --- /dev/null +++ b/test/lib/mailgun_logger/roles/roles_test.exs @@ -0,0 +1,46 @@ +defmodule MailgunLogger.RolesTest do + use MailgunLogger.DataCase + + alias MailgunLogger.Roles + alias MailgunLogger.User + + defp user_with_roles(id, roles) do + %User{ + id: id, + roles: Enum.map(roles, &%{name: &1}) + } + end + + test "returns false when user tries to modify themselves" do + user = user_with_roles(1, ["admin"]) + + refute Roles.can_modify_roles?(user, user) + end + + test "returns true when user has assign_roles permission" do + user = user_with_roles(1, ["admin"]) + target = user_with_roles(2, ["member"]) + + assert Roles.can_modify_roles?(user, target) + end + + test "returns true when user has manage_admins permission" do + user = user_with_roles(1, ["superuser"]) + target = user_with_roles(2, ["admin"]) + + assert Roles.can_modify_roles?(user, target) + end + + test "returns false when user has no relevant roles" do + user = user_with_roles(1, ["member"]) + target = user_with_roles(2, ["member"]) + + refute Roles.can_modify_roles?(user, target) + end + + test "returns false when user has no roles" do + user = %User{id: 1, roles: []} + target = %User{id: 2, roles: [%{name: "member"}]} + refute Roles.can_modify_roles?(user, target) + end +end