From a20cb3965242f378b017c6966e971ad06907574d Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Fri, 16 Aug 2024 10:59:31 +0200 Subject: [PATCH] Unify and refactor login regardless of trigger source (explicit/register) (#4434) * Unify and refactor login regardless of trigger source (explicit or registration) * Fix code formatting --- lib/plausible/auth/auth.ex | 18 ++ lib/plausible/site/admin.ex | 67 ++++--- .../site/memberships/create_invitation.ex | 2 +- .../controllers/auth_controller.ex | 166 ++++++++---------- lib/plausible_web/live/register_form.ex | 15 +- lib/plausible_web/router.ex | 2 - .../templates/page/index.html.eex | 2 +- .../controllers/auth_controller_test.exs | 43 ++--- .../plausible_web/live/register_form_test.exs | 13 ++ 9 files changed, 174 insertions(+), 154 deletions(-) diff --git a/lib/plausible/auth/auth.ex b/lib/plausible/auth/auth.ex index 65044fc2a..4a65950f1 100644 --- a/lib/plausible/auth/auth.ex +++ b/lib/plausible/auth/auth.ex @@ -50,10 +50,28 @@ defmodule Plausible.Auth do |> Repo.insert() end + @spec find_user_by(Keyword.t()) :: Auth.User.t() | nil def find_user_by(opts) do Repo.get_by(Auth.User, opts) end + @spec get_user_by(Keyword.t()) :: {:ok, Auth.User.t()} | {:error, :user_not_found} + def get_user_by(opts) do + case Repo.get_by(Auth.User, opts) do + %Auth.User{} = user -> {:ok, user} + nil -> {:error, :user_not_found} + end + end + + @spec check_password(Auth.User.t(), String.t()) :: :ok | {:error, :wrong_password} + def check_password(user, password) do + if Plausible.Auth.Password.match?(password, user.password_hash || "") do + :ok + else + {:error, :wrong_password} + end + end + def has_active_sites?(user, roles \\ [:owner, :admin, :viewer]) do sites = Repo.all( diff --git a/lib/plausible/site/admin.ex b/lib/plausible/site/admin.ex index a22be106e..6b87e5f90 100644 --- a/lib/plausible/site/admin.ex +++ b/lib/plausible/site/admin.ex @@ -106,28 +106,24 @@ defmodule Plausible.SiteAdmin do end defp transfer_ownership(conn, sites, %{"email" => email}) do - new_owner = Plausible.Auth.find_user_by(email: email) - inviter = conn.assigns[:current_user] + inviter = conn.assigns.current_user - if new_owner do - result = - Plausible.Site.Memberships.bulk_create_invitation( - sites, - inviter, - new_owner.email, - :owner, - check_permissions: false - ) - - case result do - {:ok, _} -> - :ok - - {:error, :transfer_to_self} -> - {:error, "User is already an owner of one of the sites"} - end + with {:ok, new_owner} <- Plausible.Auth.get_user_by(email: email), + {:ok, _} <- + Plausible.Site.Memberships.bulk_create_invitation( + sites, + inviter, + new_owner.email, + :owner, + check_permissions: false + ) do + :ok else - {:error, "User could not be found"} + {:error, :user_not_found} -> + {:error, "User could not be found"} + + {:error, :transfer_to_self} -> + {:error, "User is already an owner of one of the sites"} end end @@ -136,24 +132,21 @@ defmodule Plausible.SiteAdmin do end defp transfer_ownership_direct(_conn, sites, %{"email" => email}) do - new_owner = Plausible.Auth.find_user_by(email: email) - - if new_owner do - case Plausible.Site.Memberships.bulk_transfer_ownership_direct(sites, new_owner) do - {:ok, _} -> - :ok - - {:error, :transfer_to_self} -> - {:error, "User is already an owner of one of the sites"} - - {:error, :no_plan} -> - {:error, "The new owner does not have a subscription"} - - {:error, {:over_plan_limits, limits}} -> - {:error, "Plan limits exceeded for one of the sites: #{Enum.join(limits, ", ")}"} - end + with {:ok, new_owner} <- Plausible.Auth.get_user_by(email: email), + {:ok, _} <- Plausible.Site.Memberships.bulk_transfer_ownership_direct(sites, new_owner) do + :ok else - {:error, "User could not be found"} + {:error, :user_not_found} -> + {:error, "User could not be found"} + + {:error, :transfer_to_self} -> + {:error, "User is already an owner of one of the sites"} + + {:error, :no_plan} -> + {:error, "The new owner does not have a subscription"} + + {:error, {:over_plan_limits, limits}} -> + {:error, "Plan limits exceeded for one of the sites: #{Enum.join(limits, ", ")}"} end end diff --git a/lib/plausible/site/memberships/create_invitation.ex b/lib/plausible/site/memberships/create_invitation.ex index 69b4cbd83..615fd3b8f 100644 --- a/lib/plausible/site/memberships/create_invitation.ex +++ b/lib/plausible/site/memberships/create_invitation.ex @@ -74,7 +74,7 @@ defmodule Plausible.Site.Memberships.CreateInvitation do with site <- Plausible.Repo.preload(site, :owner), :ok <- check_invitation_permissions(site, inviter, role, opts), :ok <- check_team_member_limit(site, role, invitee_email), - invitee <- Plausible.Auth.find_user_by(email: invitee_email), + invitee = Plausible.Auth.find_user_by(email: invitee_email), :ok <- Invitations.ensure_transfer_valid(site, invitee, role), :ok <- ensure_new_membership(site, invitee, role), %Ecto.Changeset{} = changeset <- Invitation.new(attrs), diff --git a/lib/plausible_web/controllers/auth_controller.ex b/lib/plausible_web/controllers/auth_controller.ex index 89ee79ca4..ac800d9ad 100644 --- a/lib/plausible_web/controllers/auth_controller.ex +++ b/lib/plausible_web/controllers/auth_controller.ex @@ -54,36 +54,11 @@ defmodule PlausibleWeb.AuthController do ] ) + # Plug purging 2FA user session cookie outsite 2FA flow defp clear_2fa_user(conn, _opts) do TwoFactor.Session.clear_2fa_user(conn) end - def register(conn, %{"user" => %{"email" => email, "password" => password}}) do - with {:ok, user} <- login_user(conn, email, password) do - conn = set_user_session(conn, user) - - if user.email_verified do - redirect(conn, to: Routes.site_path(conn, :new, flow: "register")) - else - Auth.EmailVerification.issue_code(user) - redirect(conn, to: Routes.auth_path(conn, :activate_form, flow: "register")) - end - end - end - - def register_from_invitation(conn, %{"user" => %{"email" => email, "password" => password}}) do - with {:ok, user} <- login_user(conn, email, password) do - conn = set_user_session(conn, user) - - if user.email_verified do - redirect(conn, to: Routes.site_path(conn, :index)) - else - Auth.EmailVerification.issue_code(user) - redirect(conn, to: Routes.auth_path(conn, :activate_form, flow: "invitation")) - end - end - end - def activate_form(conn, params) do user = conn.assigns.current_user flow = params["flow"] || "register" @@ -225,26 +200,47 @@ defmodule PlausibleWeb.AuthController do |> redirect(to: Routes.auth_path(conn, :login_form)) end - def login(conn, %{"email" => email, "password" => password}) do - with {:ok, user} <- login_user(conn, email, password) do - if Auth.TOTP.enabled?(user) and not TwoFactor.Session.remember_2fa?(conn, user) do - conn - |> TwoFactor.Session.set_2fa_user(user) - |> redirect(to: Routes.auth_path(conn, :verify_2fa)) - else - set_user_session_and_redirect(conn, user) - end - end + def login_form(conn, _params) do + render(conn, "login_form.html", layout: {PlausibleWeb.LayoutView, "focus.html"}) end - defp login_user(conn, email, password) do + def login(conn, %{"user" => params}) do + login(conn, params) + end + + def login(conn, %{"email" => email, "password" => password} = params) do with :ok <- Auth.rate_limit(:login_ip, conn), - {:ok, user} <- find_user(email), + {:ok, user} <- Auth.get_user_by(email: email), :ok <- Auth.rate_limit(:login_user, user), - :ok <- check_password(user, password) do - {:ok, user} + :ok <- Auth.check_password(user, password), + :ok <- check_2fa_verified(conn, user) do + conn = + cond do + not is_nil(params["register_action"]) and not user.email_verified -> + Auth.EmailVerification.issue_code(user) + + flow = + if params["register_action"] == "register_form" do + "register" + else + "invitation" + end + + put_session(conn, :login_dest, Routes.auth_path(conn, :activate_form, flow: flow)) + + params["register_action"] == "register_from_invitation_form" -> + put_session(conn, :login_dest, Routes.site_path(conn, :index)) + + params["register_action"] == "register_form" -> + put_session(conn, :login_dest, Routes.site_path(conn, :new)) + + true -> + conn + end + + set_user_session_and_redirect(conn, user) else - :wrong_password -> + {:error, :wrong_password} -> maybe_log_failed_login_attempts("wrong password for #{email}") render(conn, "login_form.html", @@ -252,7 +248,7 @@ defmodule PlausibleWeb.AuthController do layout: {PlausibleWeb.LayoutView, "focus.html"} ) - :user_not_found -> + {:error, :user_not_found} -> maybe_log_failed_login_attempts("user not found for #{email}") Plausible.Auth.Password.dummy_calculation() @@ -269,61 +265,22 @@ defmodule PlausibleWeb.AuthController do 429, "Too many login attempts. Wait a minute before trying again." ) + + {:error, {:unverified_2fa, user}} -> + conn + |> TwoFactor.Session.set_2fa_user(user) + |> redirect(to: Routes.auth_path(conn, :verify_2fa)) end end - defp redirect_to_login(conn) do - redirect(conn, to: Routes.auth_path(conn, :login_form)) - end - - defp set_user_session_and_redirect(conn, user) do - login_dest = get_session(conn, :login_dest) || Routes.site_path(conn, :index) - - conn - |> set_user_session(user) - |> put_session(:login_dest, nil) - |> redirect(external: login_dest) - end - - defp set_user_session(conn, user) do - conn - |> TwoFactor.Session.clear_2fa_user() - |> put_session(:current_user_id, user.id) - |> put_resp_cookie("logged_in", "true", - http_only: false, - max_age: 60 * 60 * 24 * 365 * 5000 - ) - end - - defp maybe_log_failed_login_attempts(message) do - if Application.get_env(:plausible, :log_failed_login_attempts) do - Logger.warning("[login] #{message}") - end - end - - defp find_user(email) do - user = - Repo.one( - from(u in Plausible.Auth.User, - where: u.email == ^email - ) - ) - - if user, do: {:ok, user}, else: :user_not_found - end - - defp check_password(user, password) do - if Plausible.Auth.Password.match?(password, user.password_hash || "") do - :ok + defp check_2fa_verified(conn, user) do + if Auth.TOTP.enabled?(user) and not TwoFactor.Session.remember_2fa?(conn, user) do + {:error, {:unverified_2fa, user}} else - :wrong_password + :ok end end - def login_form(conn, _params) do - render(conn, "login_form.html", layout: {PlausibleWeb.LayoutView, "focus.html"}) - end - def user_settings(conn, _params) do user = conn.assigns.current_user settings_changeset = Auth.User.settings_changeset(user) @@ -750,4 +707,33 @@ defmodule PlausibleWeb.AuthController do redirect(conn, external: "/#{URI.encode_www_form(site.domain)}/settings/integrations") end end + + defp redirect_to_login(conn) do + redirect(conn, to: Routes.auth_path(conn, :login_form)) + end + + defp set_user_session_and_redirect(conn, user) do + login_dest = get_session(conn, :login_dest) || Routes.site_path(conn, :index) + + conn + |> set_user_session(user) + |> put_session(:login_dest, nil) + |> redirect(external: login_dest) + end + + defp set_user_session(conn, user) do + conn + |> TwoFactor.Session.clear_2fa_user() + |> put_session(:current_user_id, user.id) + |> put_resp_cookie("logged_in", "true", + http_only: false, + max_age: 60 * 60 * 24 * 365 * 5000 + ) + end + + defp maybe_log_failed_login_attempts(message) do + if Application.get_env(:plausible, :log_failed_login_attempts) do + Logger.warning("[login] #{message}") + end + end end diff --git a/lib/plausible_web/live/register_form.ex b/lib/plausible_web/live/register_form.ex index fca4fca34..ab939e1d3 100644 --- a/lib/plausible_web/live/register_form.ex +++ b/lib/plausible_web/live/register_form.ex @@ -34,6 +34,7 @@ defmodule PlausibleWeb.Live.RegisterForm do form: to_form(changeset), captcha_error: nil, password_strength: Auth.User.password_strength(changeset), + disable_submit: false, trigger_submit: false )} end @@ -53,7 +54,7 @@ defmodule PlausibleWeb.Live.RegisterForm do Your invitation has expired or been revoked. Please request fresh one or you can <%= link( "sign up", class: "text-indigo-600 hover:text-indigo-900", - to: Routes.auth_path(@socket, :register) + to: Routes.auth_path(@socket, :register_form) ) %> for a 30-day unlimited free trial without an invitation.

@@ -91,13 +92,14 @@ defmodule PlausibleWeb.Live.RegisterForm do :let={f} for={@form} id="register-form" + action={Routes.auth_path(@socket, :login)} phx-hook="Metrics" phx-change="validate" phx-submit="register" phx-trigger-action={@trigger_submit} class="w-full max-w-md mx-auto bg-white dark:bg-gray-800 shadow-md rounded px-8 py-6 mb-4 mt-8" > - +

Enter your details

@@ -178,7 +180,12 @@ defmodule PlausibleWeb.Live.RegisterForm do else "Start my free trial →" end %> - + <%= submit_text %> @@ -318,6 +325,8 @@ defmodule PlausibleWeb.Live.RegisterForm do defp add_user(socket, user) do case Repo.insert(user) do {:ok, _user} -> + socket = assign(socket, disable_submit: true) + on_ee do event_name = "Signup#{if socket.assigns.invitation, do: " via invitation"}" {:noreply, push_event(socket, "send-metrics", %{event_name: event_name})} diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 3857c4478..8404bf9d8 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -246,8 +246,6 @@ defmodule PlausibleWeb.Router do end end - post "/register", AuthController, :register - post "/register/invitation/:invitation_id", AuthController, :register_from_invitation get "/activate", AuthController, :activate_form post "/activate/request-code", AuthController, :request_activation_code post "/activate", AuthController, :activate diff --git a/lib/plausible_web/templates/page/index.html.eex b/lib/plausible_web/templates/page/index.html.eex index 5104bdf94..a100a164c 100644 --- a/lib/plausible_web/templates/page/index.html.eex +++ b/lib/plausible_web/templates/page/index.html.eex @@ -13,7 +13,7 @@ <% end %>
  • - <%= link to: Routes.auth_path(@conn, :register), class: "flex items-center" do %> + <%= link to: Routes.auth_path(@conn, :register_form), class: "flex items-center" do %> Register <% end %> diff --git a/test/plausible_web/controllers/auth_controller_test.exs b/test/plausible_web/controllers/auth_controller_test.exs index f2a75faca..6f57f8e59 100644 --- a/test/plausible_web/controllers/auth_controller_test.exs +++ b/test/plausible_web/controllers/auth_controller_test.exs @@ -28,7 +28,7 @@ defmodule PlausibleWeb.AuthControllerTest do end end - describe "POST /register" do + describe "POST /login (register_action = register_form)" do test "registering sends an activation link", %{conn: conn} do Repo.insert!( User.new(%{ @@ -39,10 +39,11 @@ defmodule PlausibleWeb.AuthControllerTest do }) ) - post(conn, "/register", + post(conn, "/login", user: %{ email: "user@example.com", - password: "very-secret-and-very-long-123" + password: "very-secret-and-very-long-123", + register_action: "register_form" } ) @@ -62,10 +63,11 @@ defmodule PlausibleWeb.AuthControllerTest do ) conn = - post(conn, "/register", + post(conn, "/login", user: %{ email: "user@example.com", - password: "very-secret-and-very-long-123" + password: "very-secret-and-very-long-123", + register_action: "register_form" } ) @@ -83,10 +85,11 @@ defmodule PlausibleWeb.AuthControllerTest do ) conn = - post(conn, "/register", + post(conn, "/login", user: %{ email: "user@example.com", - password: "very-secret-and-very-long-123" + password: "very-secret-and-very-long-123", + register_action: "register_form" } ) @@ -113,7 +116,7 @@ defmodule PlausibleWeb.AuthControllerTest do end end - describe "POST /register/invitation/:invitation_id" do + describe "POST /login (register_action = register_from_invitation_form)" do setup do inviter = insert(:user) site = insert(:site, members: [inviter]) @@ -138,13 +141,14 @@ defmodule PlausibleWeb.AuthControllerTest do {:ok, %{site: site, invitation: invitation}} end - test "registering sends an activation link", %{conn: conn, invitation: invitation} do - post(conn, "/register/invitation/#{invitation.invitation_id}", + test "registering sends an activation link", %{conn: conn} do + post(conn, "/login", user: %{ name: "Jane Doe", email: "user@example.com", password: "very-secret-and-very-long-123", - password_confirmation: "very-secret-and-very-long-123" + password_confirmation: "very-secret-and-very-long-123", + register_action: "register_from_invitation_form" } ) @@ -153,31 +157,30 @@ defmodule PlausibleWeb.AuthControllerTest do assert subject =~ "is your Plausible email verification code" end - test "user is redirected to activate page after registration", %{ - conn: conn, - invitation: invitation - } do + test "user is redirected to activate page after registration", %{conn: conn} do conn = - post(conn, "/register/invitation/#{invitation.invitation_id}", + post(conn, "/login", user: %{ name: "Jane Doe", email: "user@example.com", password: "very-secret-and-very-long-123", - password_confirmation: "very-secret-and-very-long-123" + password_confirmation: "very-secret-and-very-long-123", + register_action: "register_from_invitation_form" } ) assert redirected_to(conn, 302) == "/activate?flow=invitation" end - test "logs the user in", %{conn: conn, invitation: invitation} do + test "logs the user in", %{conn: conn} do conn = - post(conn, "/register/invitation/#{invitation.invitation_id}", + post(conn, "/login", user: %{ name: "Jane Doe", email: "user@example.com", password: "very-secret-and-very-long-123", - password_confirmation: "very-secret-and-very-long-123" + password_confirmation: "very-secret-and-very-long-123", + register_action: "register_from_invitation_form" } ) diff --git a/test/plausible_web/live/register_form_test.exs b/test/plausible_web/live/register_form_test.exs index 24e92db49..c946140e1 100644 --- a/test/plausible_web/live/register_form_test.exs +++ b/test/plausible_web/live/register_form_test.exs @@ -69,6 +69,7 @@ defmodule PlausibleWeb.Live.RegisterFormTest do assert [ csrf_input, + action_input, name_input, email_input, password_input, @@ -76,6 +77,7 @@ defmodule PlausibleWeb.Live.RegisterFormTest do ] = find(html, "input") assert String.length(text_of_attr(csrf_input, "value")) > 0 + assert text_of_attr(action_input, "value") == "register_form" assert text_of_attr(name_input, "value") == "Mary Sue" assert text_of_attr(email_input, "value") == "mary.sue@plausible.test" assert text_of_attr(password_input, "value") == "very-long-and-very-secret-123" @@ -167,6 +169,7 @@ defmodule PlausibleWeb.Live.RegisterFormTest do assert [ csrf_input, + action_input, email_input, name_input, password_input, @@ -174,6 +177,7 @@ defmodule PlausibleWeb.Live.RegisterFormTest do ] = find(html, "input") assert String.length(text_of_attr(csrf_input, "value")) > 0 + assert text_of_attr(action_input, "value") == "register_from_invitation_form" assert text_of_attr(name_input, "value") == "Mary Sue" assert text_of_attr(email_input, "value") == "user@email.co" assert text_of_attr(password_input, "value") == "very-long-and-very-secret-123" @@ -235,6 +239,7 @@ defmodule PlausibleWeb.Live.RegisterFormTest do assert [ _csrf_input, + _action_input, email_input | _ ] = find(html, "input") @@ -245,6 +250,14 @@ defmodule PlausibleWeb.Live.RegisterFormTest do refute Repo.get_by(User, email: "mary.sue@plausible.test") end + test "renders expired invitation notice on on-existent invitation ID", %{conn: conn} do + lv = get_liveview(conn, "/register/invitation/doesnotexist") + + html = render(lv) + + assert html =~ "Your invitation has expired or been revoked" + end + test "renders error on failed captcha", %{conn: conn, invitation: invitation} do mock_captcha_failure()