From 36eef5f0a9a79c3d438b3dc75497a6e01536b9c7 Mon Sep 17 00:00:00 2001 From: Rui Oliveira <70754369+ruioliveira02@users.noreply.github.com> Date: Sat, 12 Aug 2023 16:24:32 +0100 Subject: [PATCH] Update email confirmation (#307) * Confirmation page * Confirmation email * Add confirmed at plug * Formatting * Fix tests * Suggestions --- lib/atomic/accounts/user_notifier.ex | 25 ++++++++--------- lib/atomic_web/controllers/user_auth.ex | 13 +++++++++ .../user_confirmation_controller.ex | 8 +++--- .../user_registration_controller.ex | 5 ++-- .../controllers/user_session_controller.ex | 9 +++++-- lib/atomic_web/router.ex | 8 +++--- .../templates/email/user_confirmation.txt.eex | 7 +++++ .../user_confirmation/edit.html.heex | 27 ++++++++++++------- test/atomic/organizations_test.exs | 2 +- .../user_confirmation_controller_test.exs | 8 +++--- .../user_registration_controller_test.exs | 12 ++------- .../user_session_controller_test.exs | 7 +++-- test/support/conn_case.ex | 4 ++- 13 files changed, 80 insertions(+), 55 deletions(-) create mode 100644 lib/atomic_web/templates/email/user_confirmation.txt.eex diff --git a/lib/atomic/accounts/user_notifier.ex b/lib/atomic/accounts/user_notifier.ex index 30aa3a487..b813f76f2 100644 --- a/lib/atomic/accounts/user_notifier.ex +++ b/lib/atomic/accounts/user_notifier.ex @@ -8,8 +8,8 @@ defmodule Atomic.Accounts.UserNotifier do defp base_email(to: email) do new() - |> from({"Atomic", "noreply@atomic.cesium.pt"}) |> to(email) + |> from({"Atomic", "noreply@atomic.cesium.pt"}) end defp deliver(recipient, subject, body) do @@ -29,20 +29,17 @@ defmodule Atomic.Accounts.UserNotifier do Deliver instructions to confirm account. """ def deliver_confirmation_instructions(user, url) do - deliver(user.email, "Confirmation instructions", """ - - ============================== - - Hi #{user.email}, - - You can confirm your account by visiting the URL below: - - #{url} - - If you didn't create an account with us, please ignore this. + email = + base_email(to: user.email) + |> subject("Confirm your Account") + |> assign(:user, user) + |> assign(:url, url) + |> render_body("user_confirmation.txt") - ============================== - """) + case Mailer.deliver(email) do + {:ok, _term} -> {:ok, email} + {:error, ch} -> {:error, ch} + end end @doc """ diff --git a/lib/atomic_web/controllers/user_auth.ex b/lib/atomic_web/controllers/user_auth.ex index 5e1b9ea2d..f7dd39090 100644 --- a/lib/atomic_web/controllers/user_auth.ex +++ b/lib/atomic_web/controllers/user_auth.ex @@ -160,6 +160,19 @@ defmodule AtomicWeb.UserAuth do end end + def require_confirmed_user(conn, _opts) do + current_user = conn.assigns[:current_user] + + if conn.assigns[:current_user] && not is_nil(current_user.confirmed_at) do + conn + else + conn + |> put_flash(:error, "You must confirm your account in order to access this page.") + |> redirect(to: "/404") + |> halt() + end + end + defp maybe_store_return_to(%{method: "GET"} = conn) do put_session(conn, :user_return_to, current_path(conn)) end diff --git a/lib/atomic_web/controllers/user_confirmation_controller.ex b/lib/atomic_web/controllers/user_confirmation_controller.ex index 1121b9f81..db7e43730 100644 --- a/lib/atomic_web/controllers/user_confirmation_controller.ex +++ b/lib/atomic_web/controllers/user_confirmation_controller.ex @@ -4,7 +4,7 @@ defmodule AtomicWeb.UserConfirmationController do alias Atomic.Accounts def new(conn, _params) do - render(conn, "new.html") + render(conn, "new.html", error_message: nil) end def create(conn, %{"user" => %{"email" => email}}) do @@ -25,7 +25,7 @@ defmodule AtomicWeb.UserConfirmationController do end def edit(conn, %{"token" => token}) do - render(conn, "edit.html", token: token) + render(conn, "edit.html", token: token, error_message: nil) end # Do not log in the user after confirmation to avoid a @@ -35,7 +35,7 @@ defmodule AtomicWeb.UserConfirmationController do {:ok, _} -> conn |> put_flash(:info, "User confirmed successfully.") - |> redirect(to: "/") + |> redirect(to: "/users/log_in") :error -> # If there is a current user and the account was already confirmed, @@ -49,7 +49,7 @@ defmodule AtomicWeb.UserConfirmationController do %{} -> conn |> put_flash(:error, "User confirmation link is invalid or it has expired.") - |> redirect(to: "/") + |> redirect(to: "/users/log_in") end end end diff --git a/lib/atomic_web/controllers/user_registration_controller.ex b/lib/atomic_web/controllers/user_registration_controller.ex index 094b4ba24..dfba04491 100644 --- a/lib/atomic_web/controllers/user_registration_controller.ex +++ b/lib/atomic_web/controllers/user_registration_controller.ex @@ -3,7 +3,6 @@ defmodule AtomicWeb.UserRegistrationController do alias Atomic.Accounts alias Atomic.Accounts.User - alias AtomicWeb.UserAuth def new(conn, _params) do changeset = Accounts.change_user_registration(%User{}) @@ -25,8 +24,8 @@ defmodule AtomicWeb.UserRegistrationController do ) conn - |> put_flash(:info, "User created successfully.") - |> UserAuth.log_in_user(user) + |> put_flash(:info, "Registered successfully. Check your email inbox before continuing") + |> render("new.html", changeset: Accounts.change_user_registration(user)) {:error, %Ecto.Changeset{} = changeset} -> render(conn, "new.html", changeset: changeset) diff --git a/lib/atomic_web/controllers/user_session_controller.ex b/lib/atomic_web/controllers/user_session_controller.ex index 13d2ed0eb..c80a40b5e 100644 --- a/lib/atomic_web/controllers/user_session_controller.ex +++ b/lib/atomic_web/controllers/user_session_controller.ex @@ -10,9 +10,14 @@ defmodule AtomicWeb.UserSessionController do def create(conn, %{"user" => user_params}) do %{"email" => email, "password" => password} = user_params + user = Accounts.get_user_by_email_and_password(email, password) - if user = Accounts.get_user_by_email_and_password(email, password) do - UserAuth.log_in_user(conn, user, user_params) + if user do + if is_nil(user.confirmed_at) do + render(conn, "new.html", error_message: "Confirm your email before continuing") + else + UserAuth.log_in_user(conn, user, user_params) + end else # In order to prevent user enumeration attacks, don't disclose whether the email is registered. render(conn, "new.html", error_message: "Invalid email or password") diff --git a/lib/atomic_web/router.ex b/lib/atomic_web/router.ex index 31be90739..b9993ec15 100644 --- a/lib/atomic_web/router.ex +++ b/lib/atomic_web/router.ex @@ -44,7 +44,10 @@ defmodule AtomicWeb.Router do end scope "/", AtomicWeb do - pipe_through [:browser, :require_authenticated_user] + pipe_through [:browser, :require_authenticated_user, :require_confirmed_user] + + get "/users/settings", UserSettingsController, :edit + put "/users/settings", UserSettingsController, :update live_session :logged_in, on_mount: [{AtomicWeb.Hooks, :authenticated_user_state}] do live "/", HomeLive.Index, :index @@ -155,9 +158,6 @@ defmodule AtomicWeb.Router do scope "/", AtomicWeb do pipe_through [:browser, :require_authenticated_user] - - get "/users/settings", UserSettingsController, :edit - put "/users/settings", UserSettingsController, :update get "/users/settings/confirm_email/:token", UserSettingsController, :confirm_email end diff --git a/lib/atomic_web/templates/email/user_confirmation.txt.eex b/lib/atomic_web/templates/email/user_confirmation.txt.eex new file mode 100644 index 000000000..057d56aff --- /dev/null +++ b/lib/atomic_web/templates/email/user_confirmation.txt.eex @@ -0,0 +1,7 @@ +Hi <%= @user.email %>, + +You can confirm your Atomic account by visiting the URL below: + +<%= @url %> + +If you didn't create an account with us, please ignore this. diff --git a/lib/atomic_web/templates/user_confirmation/edit.html.heex b/lib/atomic_web/templates/user_confirmation/edit.html.heex index fde4d93d3..2cc47cf18 100644 --- a/lib/atomic_web/templates/user_confirmation/edit.html.heex +++ b/lib/atomic_web/templates/user_confirmation/edit.html.heex @@ -1,11 +1,18 @@ -

Confirm account

+
+ <.form let={_f} for={:user} action={Routes.user_confirmation_path(@conn, :update, @token)} as={:user} class="flex flex-col items-center justify-center w-full h-full mx-8 mt-8 space-y-6 sm:mx-0"> +

+ <%= gettext("Confirm your Account") %> +

+ <%= if @error_message do %> +
+

<%= @error_message %>

+
+ <% end %> -<.form let={_f} for={:user} action={Routes.user_confirmation_path(@conn, :update, @token)}> -
- <%= submit("Confirm my account") %> -
- - -

- <%= link("Register", to: Routes.user_registration_path(@conn, :new)) %> | <%= link("Log in", to: Routes.user_session_path(@conn, :new)) %> -

+
+ <%= submit class: "w-64 border-2 rounded-md bg-orange-500 text-lg border-orange-500 py-2 px-3.5 text-sm font-medium text-white shadow-sm" do %> + <%= gettext("Confirm") %> + <% end %> +
+ +
diff --git a/test/atomic/organizations_test.exs b/test/atomic/organizations_test.exs index 472f678bb..1a7b89191 100644 --- a/test/atomic/organizations_test.exs +++ b/test/atomic/organizations_test.exs @@ -74,7 +74,7 @@ defmodule Atomic.OrganizationsTest do alias Atomic.Organizations.Membership test "list_memberships/1 returns all memberships of organization" do - membership = insert(:membership) + membership = insert(:membership, role: :member) memberships = Organizations.list_memberships(%{"organization_id" => membership.organization_id}) diff --git a/test/atomic_web/controllers/user_confirmation_controller_test.exs b/test/atomic_web/controllers/user_confirmation_controller_test.exs index 348539aaa..ae1c65501 100644 --- a/test/atomic_web/controllers/user_confirmation_controller_test.exs +++ b/test/atomic_web/controllers/user_confirmation_controller_test.exs @@ -59,7 +59,7 @@ defmodule AtomicWeb.UserConfirmationControllerTest do test "renders the confirmation page", %{conn: conn} do conn = get(conn, Routes.user_confirmation_path(conn, :edit, "some-token")) response = html_response(conn, 200) - assert response =~ "

Confirm account

" + assert response =~ "Confirm your Account" form_action = Routes.user_confirmation_path(conn, :update, "some-token") assert response =~ "action=\"#{form_action}\"" @@ -74,7 +74,7 @@ defmodule AtomicWeb.UserConfirmationControllerTest do end) conn = post(conn, Routes.user_confirmation_path(conn, :update, token)) - assert redirected_to(conn) == "/" + assert redirected_to(conn) == "/users/log_in" assert get_flash(conn, :info) =~ "User confirmed successfully" assert Accounts.get_user!(user.id).confirmed_at refute get_session(conn, :user_token) @@ -82,7 +82,7 @@ defmodule AtomicWeb.UserConfirmationControllerTest do # When not logged in conn = post(conn, Routes.user_confirmation_path(conn, :update, token)) - assert redirected_to(conn) == "/" + assert redirected_to(conn) == "/users/log_in" assert get_flash(conn, :error) =~ "User confirmation link is invalid or it has expired" # When logged in @@ -97,7 +97,7 @@ defmodule AtomicWeb.UserConfirmationControllerTest do test "does not confirm email with invalid token", %{conn: conn, user: user} do conn = post(conn, Routes.user_confirmation_path(conn, :update, "oops")) - assert redirected_to(conn) == "/" + assert redirected_to(conn) == "/users/log_in" assert get_flash(conn, :error) =~ "User confirmation link is invalid or it has expired" refute Accounts.get_user!(user.id).confirmed_at end diff --git a/test/atomic_web/controllers/user_registration_controller_test.exs b/test/atomic_web/controllers/user_registration_controller_test.exs index e2bcd674f..017c73ba6 100644 --- a/test/atomic_web/controllers/user_registration_controller_test.exs +++ b/test/atomic_web/controllers/user_registration_controller_test.exs @@ -18,7 +18,7 @@ defmodule AtomicWeb.UserRegistrationControllerTest do describe "POST /users/register" do @tag :capture_log - test "creates account and logs the user in", %{conn: conn} do + test "creates account but doesn't log the user in", %{conn: conn} do organization = insert(:organization) user_attrs = %{ @@ -34,15 +34,7 @@ defmodule AtomicWeb.UserRegistrationControllerTest do "user" => user_attrs }) - assert get_session(conn, :user_token) - assert redirected_to(conn) == "/" - - # Now do a logged in request and assert on the menu - conn = get(conn, "/") - - response = html_response(conn, 200) - - assert response =~ "Home" + assert is_nil(get_session(conn, :user_token)) end end end diff --git a/test/atomic_web/controllers/user_session_controller_test.exs b/test/atomic_web/controllers/user_session_controller_test.exs index 3487f5a82..d2d8554f4 100644 --- a/test/atomic_web/controllers/user_session_controller_test.exs +++ b/test/atomic_web/controllers/user_session_controller_test.exs @@ -5,7 +5,11 @@ defmodule AtomicWeb.UserSessionControllerTest do setup do organization = insert(:organization) - %{user: insert(:user, default_organization_id: organization.id)} + + %{ + user: + insert(:user, default_organization_id: organization.id, confirmed_at: DateTime.utc_now()) + } end describe "GET /users/log_in" do @@ -36,7 +40,6 @@ defmodule AtomicWeb.UserSessionControllerTest do # Now do a logged in request and assert on the menu conn = get(conn, "/") response = html_response(conn, 200) - assert response =~ user.email assert response =~ "Home" end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 5ec04b4b9..65bad9b46 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -49,7 +49,9 @@ defmodule AtomicWeb.ConnCase do """ def register_and_log_in_user(%{conn: conn}) do organization = insert(:organization) - user = insert(:user, %{default_organization_id: organization.id}) + + user = + insert(:user, %{default_organization_id: organization.id, confirmed_at: DateTime.utc_now()}) Organizations.create_membership(%{ organization_id: organization.id,