From 3852ae17b835cd00ddcbf01483d97ded9a4dd073 Mon Sep 17 00:00:00 2001 From: Marcin Kulik Date: Thu, 16 May 2024 13:48:13 +0200 Subject: [PATCH] Generate URLs for emails inside email templates, using verified routes --- lib/asciinema.ex | 16 +++-- lib/asciinema/accounts.ex | 17 ++--- lib/asciinema/emails.ex | 11 ++- lib/asciinema/emails/email.ex | 18 ++--- lib/asciinema_web.ex | 13 ---- .../controllers/login_controller.ex | 3 +- .../controllers/user_controller.ex | 2 +- .../templates/email/account_deletion.html.eex | 2 +- .../templates/email/account_deletion.text.eex | 2 +- .../templates/email/login.html.eex | 2 +- .../templates/email/login.text.eex | 2 +- .../templates/email/signup.html.eex | 2 +- .../templates/email/signup.text.eex | 2 +- test/asciinema/accounts_test.exs | 67 ++++++++----------- test/asciinema_test.exs | 33 ++++----- 15 files changed, 78 insertions(+), 114 deletions(-) diff --git a/lib/asciinema.ex b/lib/asciinema.ex index 650afab73..0c8f4a3d4 100644 --- a/lib/asciinema.ex +++ b/lib/asciinema.ex @@ -18,19 +18,21 @@ defmodule Asciinema do end end - def send_login_email(identifier, sign_up_enabled?, routes) do - case Accounts.generate_login_url(identifier, sign_up_enabled?, routes) do - {:ok, {type, url, email}} -> - Emails.send_email(type, email, url) + def send_login_email(identifier, sign_up_enabled? \\ true) + + def send_login_email(identifier, sign_up_enabled?) do + case Accounts.generate_login_token(identifier, sign_up_enabled?) do + {:ok, {type, token, email}} -> + Emails.send_email(type, email, token) {:error, _reason} = result -> result end end - def send_account_deletion_email(user, routes) do - url = Accounts.generate_deletion_url(user, routes) - Emails.send_email(:account_deletion, user.email, url) + def send_account_deletion_email(user) do + token = Accounts.generate_deletion_token(user) + Emails.send_email(:account_deletion, user.email, token) :ok end diff --git a/lib/asciinema/accounts.ex b/lib/asciinema/accounts.ex index 5b5b519fd..181116e1f 100644 --- a/lib/asciinema/accounts.ex +++ b/lib/asciinema/accounts.ex @@ -124,24 +124,23 @@ defmodule Asciinema.Accounts do from(u in q, where: is_nil(u.email)) end - def generate_login_url(identifier, sign_up_enabled?, routes) do + def generate_login_token(identifier, sign_up_enabled? \\ true) + + def generate_login_token(identifier, sign_up_enabled?) do case {lookup_user(identifier), sign_up_enabled?} do {{_, %User{email: nil}}, _} -> {:error, :email_missing} {{_, %User{} = user}, _} -> - url = user |> login_token() |> routes.login_url() - - {:ok, {:login, url, user.email}} + {:ok, {:login, login_token(user), user.email}} {{:email, nil}, true} -> changeset = change_user(%User{}, %{email: identifier}) if changeset.valid? do email = changeset.changes.email - url = email |> signup_token() |> routes.signup_url() - {:ok, {:signup, url, email}} + {:ok, {:signup, signup_token(email), email}} else {:error, :email_invalid} end @@ -209,12 +208,6 @@ defmodule Asciinema.Accounts do end end - def generate_deletion_url(user, routes) do - user - |> generate_deletion_token() - |> routes.account_deletion_url() - end - def generate_deletion_token(%User{id: user_id}) do Token.sign(config(:secret), "acct-delete", user_id) end diff --git a/lib/asciinema/emails.ex b/lib/asciinema/emails.ex index 26068a7a6..74d8826f5 100644 --- a/lib/asciinema/emails.ex +++ b/lib/asciinema/emails.ex @@ -9,17 +9,17 @@ defmodule Asciinema.Emails do case job.args["type"] do "signup" -> job.args["to"] - |> Email.signup_email(job.args["url"]) + |> Email.signup_email(job.args["token"]) |> deliver() "login" -> job.args["to"] - |> Email.login_email(job.args["url"]) + |> Email.login_email(job.args["token"]) |> deliver() "account_deletion" -> job.args["to"] - |> Email.account_deletion_email(job.args["url"]) + |> Email.account_deletion_email(job.args["token"]) |> deliver() end @@ -33,9 +33,8 @@ defmodule Asciinema.Emails do end end - def send_email(type, to, url) do - Job.new(%{type: type, to: to, url: url}) - |> Oban.insert!() + def send_email(type, to, token) do + Oban.insert!(Job.new(%{type: type, to: to, token: token})) :ok end diff --git a/lib/asciinema/emails/email.ex b/lib/asciinema/emails/email.ex index f51c41fef..ecf340268 100644 --- a/lib/asciinema/emails/email.ex +++ b/lib/asciinema/emails/email.ex @@ -2,30 +2,30 @@ defmodule Asciinema.Emails.Email do use Bamboo.Phoenix, view: AsciinemaWeb.EmailView import Bamboo.Email - def signup_email(email_address, signup_url) do + def signup_email(email_address, token) do base_email() |> to(email_address) |> subject("Welcome to #{instance_hostname()}") - |> render("signup.text", signup_url: signup_url) - |> render("signup.html", signup_url: signup_url) + |> render("signup.text", token: token) + |> render("signup.html", token: token) |> fix_text_body() end - def login_email(email_address, login_url) do + def login_email(email_address, token) do base_email() |> to(email_address) |> subject("Login to #{instance_hostname()}") - |> render("login.text", login_url: login_url) - |> render("login.html", login_url: login_url) + |> render("login.text", token: token) + |> render("login.html", token: token) |> fix_text_body() end - def account_deletion_email(email_address, confirmation_url) do + def account_deletion_email(email_address, token) do base_email() |> to(email_address) |> subject("Account deletion") - |> render("account_deletion.text", confirmation_url: confirmation_url) - |> render("account_deletion.html", confirmation_url: confirmation_url) + |> render("account_deletion.text", token: token) + |> render("account_deletion.html", token: token) |> fix_text_body() end diff --git a/lib/asciinema_web.ex b/lib/asciinema_web.ex index 39dcf4593..d5f0d0894 100644 --- a/lib/asciinema_web.ex +++ b/lib/asciinema_web.ex @@ -179,7 +179,6 @@ defmodule AsciinemaWeb do apply(__MODULE__, which, []) end - alias AsciinemaWeb.Router.Helpers, as: Routes alias AsciinemaWeb.Endpoint def instance_hostname do @@ -187,16 +186,4 @@ defmodule AsciinemaWeb do |> URI.parse() |> Map.get(:host) end - - def signup_url(token) do - Routes.users_url(Endpoint, :new, t: token) - end - - def login_url(token) do - Routes.session_url(Endpoint, :new, t: token) - end - - def account_deletion_url(token) do - Routes.user_deletion_url(Endpoint, :delete, t: token) - end end diff --git a/lib/asciinema_web/controllers/login_controller.ex b/lib/asciinema_web/controllers/login_controller.ex index 220a9e3c4..ac47d3194 100644 --- a/lib/asciinema_web/controllers/login_controller.ex +++ b/lib/asciinema_web/controllers/login_controller.ex @@ -20,8 +20,7 @@ defmodule AsciinemaWeb.LoginController do result = Asciinema.send_login_email( identifier, - Map.get(conn.assigns, :cfg_sign_up_enabled?, true), - AsciinemaWeb + Map.get(conn.assigns, :cfg_sign_up_enabled?, true) ) case result do diff --git a/lib/asciinema_web/controllers/user_controller.ex b/lib/asciinema_web/controllers/user_controller.ex index a83182bb8..95198e59d 100644 --- a/lib/asciinema_web/controllers/user_controller.ex +++ b/lib/asciinema_web/controllers/user_controller.ex @@ -152,7 +152,7 @@ defmodule AsciinemaWeb.UserController do user = conn.assigns.current_user address = user.email - :ok = Asciinema.send_account_deletion_email(user, AsciinemaWeb) + :ok = Asciinema.send_account_deletion_email(user) conn |> put_flash(:info, "Account removal initiated - check your inbox (#{address})") diff --git a/lib/asciinema_web/templates/email/account_deletion.html.eex b/lib/asciinema_web/templates/email/account_deletion.html.eex index 6bd01d02f..302e51ee7 100644 --- a/lib/asciinema_web/templates/email/account_deletion.html.eex +++ b/lib/asciinema_web/templates/email/account_deletion.html.eex @@ -2,7 +2,7 @@

If you wish to proceed, open the following link in your browser:

-

<%= @confirmation_url %>

+

"><%= url(~p"/user/delete?t=#{@token}") %>


diff --git a/lib/asciinema_web/templates/email/account_deletion.text.eex b/lib/asciinema_web/templates/email/account_deletion.text.eex index 381fc7a47..d3d5d6480 100644 --- a/lib/asciinema_web/templates/email/account_deletion.text.eex +++ b/lib/asciinema_web/templates/email/account_deletion.text.eex @@ -2,7 +2,7 @@ It seems you have requested deletion of your <%= @hostname %> account. If you wish to proceed, open the following link in your browser: -<%= @confirmation_url %> +<%= url(~p"/user/delete?t=#{@token}") %> If you did not initiate this request, just ignore this email. diff --git a/lib/asciinema_web/templates/email/login.html.eex b/lib/asciinema_web/templates/email/login.html.eex index 7814c176f..595ce5365 100644 --- a/lib/asciinema_web/templates/email/login.html.eex +++ b/lib/asciinema_web/templates/email/login.html.eex @@ -2,7 +2,7 @@

Click the following link to log in to your <%= @hostname %> account:

-

<%= @login_url %>

+

"><%= url(~p"/session/new?t=#{@token}") %>


diff --git a/lib/asciinema_web/templates/email/login.text.eex b/lib/asciinema_web/templates/email/login.text.eex index 55dae9410..89885ae64 100644 --- a/lib/asciinema_web/templates/email/login.text.eex +++ b/lib/asciinema_web/templates/email/login.text.eex @@ -2,7 +2,7 @@ Welcome back! Click the following link to log in to your <%= @hostname %> account: -<%= @login_url %> +<%= url(~p"/session/new?t=#{@token}") %> If you did not initiate this request, just ignore this email. The request will expire shortly. diff --git a/lib/asciinema_web/templates/email/signup.html.eex b/lib/asciinema_web/templates/email/signup.html.eex index 6bdb53ff9..53e99ca25 100644 --- a/lib/asciinema_web/templates/email/signup.html.eex +++ b/lib/asciinema_web/templates/email/signup.html.eex @@ -2,7 +2,7 @@

Click the following link to setup your new account:

-

<%= @signup_url %>

+

"><%= url(~p"/users/new?t=#{@token}") %>


diff --git a/lib/asciinema_web/templates/email/signup.text.eex b/lib/asciinema_web/templates/email/signup.text.eex index fb768c00b..f0205888c 100644 --- a/lib/asciinema_web/templates/email/signup.text.eex +++ b/lib/asciinema_web/templates/email/signup.text.eex @@ -2,7 +2,7 @@ Welcome to <%= @hostname %>! Click the following link to setup your new account: -<%= @signup_url %> +<%= url(~p"/users/new?t=#{@token}") %> If you did not initiate this request, just ignore this email. The request will expire shortly. diff --git a/test/asciinema/accounts_test.exs b/test/asciinema/accounts_test.exs index 22ea5bbdd..683d1f704 100644 --- a/test/asciinema/accounts_test.exs +++ b/test/asciinema/accounts_test.exs @@ -1,5 +1,5 @@ defmodule Asciinema.AccountsTest do - import Asciinema.Fixtures + import Asciinema.Factory use Asciinema.DataCase use Oban.Testing, repo: Asciinema.Repo alias Asciinema.Accounts @@ -10,81 +10,72 @@ defmodule Asciinema.AccountsTest do end test "valid token" do - token = Accounts.signup_token("test@example.com") + {:ok, {:signup, token, _email}} = Accounts.generate_login_token("test@example.com") assert {:ok, "test@example.com"} = Accounts.verify_signup_token(token) end end describe "generate_login_url/3" do - defmodule Routes do - def signup_url(_), do: "http://signup" - def login_url(_), do: "http://login" - end - test "existing user, by email" do - user = fixture(:user) + insert(:user, email: "test@example.com") - assert Accounts.generate_login_url(user.email, true, Routes) == - {:ok, {:login, "http://login", user.email}} + assert {:ok, {:login, _token, "test@example.com"}} = + Accounts.generate_login_token("test@example.com") end test "existing user, by username" do - user = fixture(:user) + insert(:user, username: "test", email: "test@example.com") - assert Accounts.generate_login_url(user.username, true, Routes) == - {:ok, {:login, "http://login", user.email}} + assert {:ok, {:login, _token, "test@example.com"}} = Accounts.generate_login_token("test") end test "non-existing user, by email" do - assert Accounts.generate_login_url("foo@example.com", true, Routes) == - {:ok, {:signup, "http://signup", "foo@example.com"}} + assert {:ok, {:signup, _token, "foo@example.com"}} = + Accounts.generate_login_token("foo@example.com") - assert Accounts.generate_login_url("foo@ex.ample.com", true, Routes) == - {:ok, {:signup, "http://signup", "foo@ex.ample.com"}} + assert {:ok, {:signup, _token, "foo@ex.ample.com"}} = + Accounts.generate_login_token("foo@ex.ample.com") end test "non-existing user, by email, when sign up is disabled" do - assert Accounts.generate_login_url("foo@example.com", false, Routes) == - {:error, :user_not_found} + assert Accounts.generate_login_token("foo@example.com", false) == {:error, :user_not_found} end test "non-existing user, by email, when email is invalid" do - assert Accounts.generate_login_url("foo@", true, Routes) == {:error, :email_invalid} - - assert Accounts.generate_login_url("foo@ex.ample..com", true, Routes) == - {:error, :email_invalid} + assert Accounts.generate_login_token("foo@") == {:error, :email_invalid} + assert Accounts.generate_login_token("foo@ex.ample..com") == {:error, :email_invalid} end test "non-existing user, by username" do - assert Accounts.generate_login_url("idontexist", true, Routes) == {:error, :user_not_found} + assert Accounts.generate_login_token("idontexist") == {:error, :user_not_found} end end describe "update_user/2" do - setup do - %{user: fixture(:user)} - end - - def success(user, attrs) do - assert {:ok, user} = Accounts.update_user(user, attrs) + test "success" do + user = insert(:user) - user - end - - test "success", %{user: user} do assert success(user, %{email: "new@one.com"}) assert success(user, %{email: "ANOTHER@ONE.COM"}).email == "another@one.com" assert success(user, %{username: "newone"}) end - def assert_validation_error(user, attrs) do - assert {:error, %Ecto.Changeset{}} = Accounts.update_user(user, attrs) - end + test "validation failures" do + user = insert(:user) - test "validation failures", %{user: user} do assert_validation_error(user, %{email: "newone.com"}) assert_validation_error(user, %{email: ""}) assert_validation_error(user, %{username: ""}) end + + defp success(user, attrs) do + assert {:ok, user} = Accounts.update_user(user, attrs) + + user + end + + defp assert_validation_error(user, attrs) do + assert {:error, %Ecto.Changeset{}} = Accounts.update_user(user, attrs) + end end end diff --git a/test/asciinema_test.exs b/test/asciinema_test.exs index d1e9c0ae2..939a29227 100644 --- a/test/asciinema_test.exs +++ b/test/asciinema_test.exs @@ -2,7 +2,7 @@ defmodule AsciinemaTest do import Asciinema.Factory use Asciinema.DataCase use Oban.Testing, repo: Asciinema.Repo - alias Asciinema.{Accounts, Recordings} + alias Asciinema.Recordings describe "create_user/1" do test "succeeds when email not taken" do @@ -15,66 +15,59 @@ defmodule AsciinemaTest do describe "create_user_from_signup_token/1" do test "succeeds when email not taken" do # TODO don't reach to Accounts - token = Accounts.signup_token("test@example.com") + {:ok, {:signup, token, _email}} = + Asciinema.Accounts.generate_login_token("test@example.com") + assert {:ok, _} = Asciinema.create_user_from_signup_token(token) end end describe "send_login_email/3" do - defmodule Routes do - def signup_url(_), do: "http://signup" - def login_url(_), do: "http://login" - end - test "existing user, by email" do insert(:user, email: "test@example.com") - assert Asciinema.send_login_email("TEST@EXAMPLE.COM", true, Routes) == :ok + assert Asciinema.send_login_email("TEST@EXAMPLE.COM") == :ok assert_enqueued( worker: Asciinema.Emails.Job, - args: %{"type" => "login", "to" => "test@example.com", "url" => "http://login"} + args: %{"type" => "login", "to" => "test@example.com"} ) end test "existing user, by username" do insert(:user, username: "foobar", email: "foobar123@example.com") - assert Asciinema.send_login_email("foobar", true, Routes) == :ok + assert Asciinema.send_login_email("foobar") == :ok assert_enqueued( worker: Asciinema.Emails.Job, - args: %{"type" => "login", "to" => "foobar123@example.com", "url" => "http://login"} + args: %{"type" => "login", "to" => "foobar123@example.com"} ) end test "non-existing user, by email" do - assert Asciinema.send_login_email("NEW@EXAMPLE.COM", true, Routes) == - :ok + assert Asciinema.send_login_email("NEW@EXAMPLE.COM") == :ok assert_enqueued( worker: Asciinema.Emails.Job, - args: %{"type" => "signup", "to" => "new@example.com", "url" => "http://signup"} + args: %{"type" => "signup", "to" => "new@example.com"} ) end test "non-existing user, by email, when sign up is disabled" do - assert Asciinema.send_login_email("new@example.com", false, Routes) == - {:error, :user_not_found} + assert Asciinema.send_login_email("new@example.com", false) == {:error, :user_not_found} refute_enqueued(worker: Asciinema.Emails.Job) end test "non-existing user, by email, when email is invalid" do - assert Asciinema.send_login_email("new@", true, Routes) == - {:error, :email_invalid} + assert Asciinema.send_login_email("new@") == {:error, :email_invalid} refute_enqueued(worker: Asciinema.Emails.Job) end test "non-existing user, by username" do - assert Asciinema.send_login_email("idontexist", true, Routes) == - {:error, :user_not_found} + assert Asciinema.send_login_email("idontexist") == {:error, :user_not_found} refute_enqueued(worker: Asciinema.Emails.Job) end