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

@@ -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(

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
 

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),

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

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.
       </p>
     </div>
@@ -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"
       >
-        <input name="_csrf_token" type="hidden" value={Plug.CSRFProtection.get_csrf_token()} />
+        <input name="user[register_action]" type="hidden" value={@live_action} />
 
         <h2 class="text-xl font-black dark:text-gray-100">Enter your details</h2>
 
@@ -178,7 +180,12 @@ defmodule PlausibleWeb.Live.RegisterForm do
           else
             "Start my free trial →"
           end %>
-        <PlausibleWeb.Components.Generic.button id="register" type="submit" class="mt-4 w-full">
+        <PlausibleWeb.Components.Generic.button
+          id="register"
+          disabled={@disable_submit}
+          type="submit"
+          class="mt-4 w-full"
+        >
           <%= submit_text %>
         </PlausibleWeb.Components.Generic.button>
 
@@ -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})}

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

lib/plausible_web/templates/page/index.html.eex

@@ -13,7 +13,7 @@
       <% end %>
     </li>
     <li class="w-full md:mt-1 md:w-1/2 md:order-3">
-      <%= 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 %>
         <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="feather feather-user-plus h-4 w-4"><path d="M16 21v-2a4 4 0 0 0-4-4H5a4 4 0 0 0-4 4v2"></path><circle cx="8.5" cy="7" r="4"></circle><line x1="20" y1="8" x2="20" y2="14"></line><line x1="23" y1="11" x2="17" y2="11"></line></svg>
         <span class="ml-2 font-semibold underline text-indigo-700 dark:text-indigo-400">Register</span>
       <% end %>

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"
           }
         )
 

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()