Trigger email reverification on change (#3388)

* Implement PoC for email reverification flow on update

* Improve user settings form and email change validation

* Expose `previous_email` in Kaffy CRM

* Improve plugs setup and remove dead action from AuthController

* Fix seeds

* Extract predicate query functions from AuthController

* Add tests

* Update CHANGELOG.md

* Rename `has_any_sites?` to `Memberships.any?` and `has_any_memberships?`

* Improve flash message on cancelling email change

* Cover one more test case for email update
This commit is contained in:
Adrian Gruntkowski 2023-10-11 10:25:00 +02:00 committed by GitHub
parent 33b5e69c64
commit 439c5014d4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 385 additions and 54 deletions

View File

@ -9,6 +9,7 @@ All notable changes to this project will be documented in this file.
- Add `custom_props.csv` to CSV export (almost the same as the old `prop_breakdown.csv`, but has different column headers, and includes props for pageviews too, not only custom events)
- Add `referrers.csv` to CSV export
- Improve password validation in registration and password reset forms
- Enforce email reverification on update
### Removed
- Removed the nested custom event property breakdown UI when filtering by a goal in Goal Conversions

View File

@ -28,6 +28,7 @@ defmodule Plausible.Auth.User do
field :trial_expiry_date, :date
field :theme, Ecto.Enum, values: [:system, :light, :dark]
field :email_verified, :boolean
field :previous_email, :string
embeds_one :grace_period, Plausible.Auth.GracePeriod, on_replace: :update
has_many :site_memberships, Plausible.Site.Membership
@ -61,6 +62,31 @@ defmodule Plausible.Auth.User do
|> unique_constraint(:email)
end
def email_changeset(user, attrs \\ %{}) do
user
|> cast(attrs, [:email, :password])
|> validate_required([:email, :password])
|> validate_email_changed()
|> check_password()
|> unique_constraint(:email)
|> put_change(:email_verified, false)
|> put_change(:previous_email, user.email)
end
def cancel_email_changeset(user) do
if user.previous_email do
user
|> change()
|> unique_constraint(:email)
|> put_change(:email_verified, true)
|> put_change(:email, user.previous_email)
|> put_change(:previous_email, nil)
else
# It shouldn't happen under normal circumstances
raise "Previous email is empty for user #{user.id} (#{user.email}) when it shouldn't."
end
end
def changeset(user, attrs \\ %{}) do
user
|> cast(attrs, [:email, :name, :email_verified, :theme, :trial_expiry_date])
@ -127,6 +153,26 @@ defmodule Plausible.Auth.User do
%{suggestions: [], warning: "", score: 3}
end
defp validate_email_changed(changeset) do
if !get_change(changeset, :email) && !changeset.errors[:email] do
add_error(changeset, :email, "can't be the same", validation: :different_email)
else
changeset
end
end
defp check_password(changeset) do
if password = get_change(changeset, :password) do
if Plausible.Auth.Password.match?(password, changeset.data.password_hash) do
changeset
else
add_error(changeset, :password, "is invalid", validation: :check_password)
end
else
changeset
end
end
defp validate_password_strength(changeset) do
if get_change(changeset, :password) != nil and password_strength(changeset).score <= 2 do
add_error(changeset, :password, "is too weak", validation: :strength)

View File

@ -12,6 +12,7 @@ defmodule Plausible.Auth.UserAdmin do
[
name: nil,
email: nil,
previous_email: nil,
trial_expiry_date: nil
]
end

View File

@ -3,9 +3,30 @@ defmodule Plausible.Site.Memberships do
API for site memberships and invitations
"""
import Ecto.Query, only: [from: 2]
alias Plausible.Repo
alias Plausible.Site.Memberships
defdelegate accept_invitation(invitation_id, user), to: Memberships.AcceptInvitation
defdelegate reject_invitation(invitation_id, user), to: Memberships.RejectInvitation
defdelegate remove_invitation(invitation_id, site), to: Memberships.RemoveInvitation
@spec any?(String.t()) :: boolean()
def any?(user_id) do
Repo.exists?(
from(m in Plausible.Site.Membership,
where: m.user_id == ^user_id
)
)
end
@spec has_any_invitations?(String.t()) :: boolean()
def has_any_invitations?(email) do
Repo.exists?(
from(i in Plausible.Auth.Invitation,
where: i.email == ^email
)
)
end
end

View File

@ -26,6 +26,15 @@ defmodule Plausible.Users do
)
end
@spec has_email_code?(String.t()) :: boolean()
def has_email_code?(user_id) do
Repo.exists?(
from(c in "email_verification_codes",
where: c.user_id == ^user_id
)
)
end
defp last_subscription_query(user_id) do
from(subscription in Plausible.Billing.Subscription,
where: subscription.user_id == ^user_id,

View File

@ -7,6 +7,7 @@ defmodule PlausibleWeb.AuthController do
plug(
PlausibleWeb.RequireLoggedOutPlug
when action in [
:register_form,
:register,
:register_from_invitation,
:login_form,
@ -19,8 +20,15 @@ defmodule PlausibleWeb.AuthController do
when action in [
:user_settings,
:save_settings,
:update_email,
:cancel_update_email,
:new_api_key,
:create_api_key,
:delete_api_key,
:delete_me,
:activate_form
:activate_form,
:activate,
:request_activation_code
]
)
@ -30,13 +38,6 @@ defmodule PlausibleWeb.AuthController do
assign(conn, :is_selfhost, Plausible.Release.selfhost?())
end
def register_form(conn, _params) do
render(conn, "register_form.html",
connect_live_socket: true,
layout: {PlausibleWeb.LayoutView, "focus.html"}
)
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)
@ -78,23 +79,10 @@ defmodule PlausibleWeb.AuthController do
def activate_form(conn, _params) do
user = conn.assigns[:current_user]
has_invitation =
Repo.exists?(
from(i in Plausible.Auth.Invitation,
where: i.email == ^user.email
)
)
has_code =
Repo.exists?(
from(c in "email_verification_codes",
where: c.user_id == ^user.id
)
)
render(conn, "activate.html",
has_pin: has_code,
has_invitation: has_invitation,
has_email_code?: Plausible.Users.has_email_code?(user.id),
has_any_invitations?: Plausible.Site.Memberships.has_any_invitations?(user.email),
has_any_memberships?: Plausible.Site.Memberships.any?(user.id),
layout: {PlausibleWeb.LayoutView, "focus.html"}
)
end
@ -102,36 +90,41 @@ defmodule PlausibleWeb.AuthController do
def activate(conn, %{"code" => code}) do
user = conn.assigns[:current_user]
has_invitation =
Repo.exists?(
from(i in Plausible.Auth.Invitation,
where: i.email == ^user.email
)
)
has_any_invitations? = Plausible.Site.Memberships.has_any_invitations?(user.email)
has_any_memberships? = Plausible.Site.Memberships.any?(user.id)
{code, ""} = Integer.parse(code)
case Auth.verify_email(user, code) do
:ok ->
if has_invitation do
redirect(conn, to: Routes.site_path(conn, :index))
else
redirect(conn, to: Routes.site_path(conn, :new))
cond do
has_any_memberships? ->
conn
|> put_flash(:success, "Email updated successfully")
|> redirect(to: Routes.auth_path(conn, :user_settings) <> "#change-email-address")
has_any_invitations? ->
redirect(conn, to: Routes.site_path(conn, :index))
true ->
redirect(conn, to: Routes.site_path(conn, :new))
end
{:error, :incorrect} ->
render(conn, "activate.html",
error: "Incorrect activation code",
has_pin: true,
has_invitation: has_invitation,
has_email_code?: true,
has_any_invitations?: has_any_invitations?,
has_any_memberships?: has_any_memberships?,
layout: {PlausibleWeb.LayoutView, "focus.html"}
)
{:error, :expired} ->
render(conn, "activate.html",
error: "Code is expired, please request another one",
has_pin: false,
has_invitation: has_invitation,
has_email_code?: false,
has_any_invitations?: has_any_invitations?,
has_any_memberships?: has_any_memberships?,
layout: {PlausibleWeb.LayoutView, "focus.html"}
)
end
@ -331,8 +324,10 @@ defmodule PlausibleWeb.AuthController do
end
def user_settings(conn, _params) do
changeset = Auth.User.settings_changeset(conn.assigns[:current_user])
render_settings(conn, changeset)
settings_changeset = Auth.User.settings_changeset(conn.assigns[:current_user])
email_changeset = Auth.User.settings_changeset(conn.assigns[:current_user])
render_settings(conn, settings_changeset: settings_changeset, email_changeset: email_changeset)
end
def save_settings(conn, %{"user" => user_params}) do
@ -345,17 +340,58 @@ defmodule PlausibleWeb.AuthController do
|> redirect(to: Routes.auth_path(conn, :user_settings))
{:error, changeset} ->
render_settings(conn, changeset)
email_changeset = Auth.User.settings_changeset(conn.assigns[:current_user])
render_settings(conn, settings_changeset: changeset, email_changeset: email_changeset)
end
end
defp render_settings(conn, changeset) do
def update_email(conn, %{"user" => user_params}) do
changes = Auth.User.email_changeset(conn.assigns[:current_user], user_params)
case Repo.update(changes) do
{:ok, user} ->
send_email_verification(user)
redirect(conn, to: Routes.auth_path(conn, :activate_form))
{:error, changeset} ->
settings_changeset = Auth.User.settings_changeset(conn.assigns[:current_user])
render_settings(conn, settings_changeset: settings_changeset, email_changeset: changeset)
end
end
def cancel_update_email(conn, _params) do
changeset = Auth.User.cancel_email_changeset(conn.assigns.current_user)
case Repo.update(changeset) do
{:ok, user} ->
conn
|> put_flash(:success, "Email changed back to #{user.email}")
|> redirect(to: Routes.auth_path(conn, :user_settings) <> "#change-email-address")
{:error, _} ->
conn
|> put_flash(
:error,
"Could not cancel email update because previous email has already been taken"
)
|> redirect(to: Routes.auth_path(conn, :activate_form))
end
end
defp render_settings(conn, opts) do
settings_changeset = Keyword.fetch!(opts, :settings_changeset)
email_changeset = Keyword.fetch!(opts, :email_changeset)
user = Plausible.Users.with_subscription(conn.assigns[:current_user])
{pageview_usage, custom_event_usage} = Plausible.Billing.usage_breakdown(user)
render(conn, "user_settings.html",
user: user |> Repo.preload(:api_keys),
changeset: changeset,
settings_changeset: settings_changeset,
email_changeset: email_changeset,
subscription: user.subscription,
invoices: Plausible.Billing.paddle_api().get_invoices(user.subscription),
theme: user.theme || "system",

View File

@ -1,6 +1,13 @@
defmodule PlausibleWeb.RequireAccountPlug do
import Plug.Conn
@unverified_email_exceptions [
["settings", "email", "cancel"],
["activate"],
["activate", "request-code"],
["me"]
]
def init(options) do
options
end
@ -14,7 +21,8 @@ defmodule PlausibleWeb.RequireAccountPlug do
|> Phoenix.Controller.redirect(to: "/login")
|> halt
not user.email_verified and conn.path_info not in [["activate"], ["me"]] ->
not user.email_verified and
conn.path_info not in @unverified_email_exceptions ->
conn
|> Phoenix.Controller.redirect(to: "/activate")
|> halt

View File

@ -187,6 +187,8 @@ defmodule PlausibleWeb.Router do
get "/logout", AuthController, :logout
get "/settings", AuthController, :user_settings
put "/settings", AuthController, :save_settings
put "/settings/email", AuthController, :update_email
post "/settings/email/cancel", AuthController, :cancel_update_email
delete "/me", AuthController, :delete_me
get "/settings/api-keys/new", AuthController, :new_api_key
post "/settings/api-keys", AuthController, :create_api_key

View File

@ -1,7 +1,13 @@
<div class="w-full max-w-3xl mt-4 mx-auto flex">
<%= if @has_pin do %>
<%= if @has_email_code? do %>
<%= form_for @conn, "/activate", [class: "w-full max-w-lg mx-auto bg-white dark:bg-gray-800 shadow-md rounded px-8 py-6 mb-4 mt-8"], fn f -> %>
<h2 class="text-xl font-black dark:text-gray-100">Activate your account</h2>
<h2 class="text-xl font-black dark:text-gray-100">
<%= if @has_any_memberships? do %>
Verify your email address
<% else %>
Activate your account
<% end %>
</h2>
<div class="mt-2 text-sm text-gray-500 dark:text-gray-200 leading-tight">
Please enter the 4-digit code we sent to <b><%= @conn.assigns[:current_user].email %></b>
@ -31,9 +37,15 @@
Entered the wrong email address?
</div>
<ul class="list-disc text-xs text-gray-500 leading-tight mt-1">
<%= if @has_any_memberships? do %>
<li>
<%= link("Change email back to", class: "underline text-indigo-600", to: "/settings/email/cancel", method: "post") %> to <%= @conn.assigns[:current_user].previous_email %>
</li>
<% else %>
<li>
<%= link("Delete this account", class: "underline text-indigo-600", to: "/me?redirect=/register", method: "delete", data: [confirm: "Deleting your account cannot be reversed. Are you sure?"]) %> and start over
</li>
<% end %>
</ul>
<% end %>
<% else %>
@ -51,7 +63,7 @@
</div>
<% end %>
<%= if !@has_invitation do %>
<%= if !@has_any_invitations? and !@has_any_memberships? do %>
<div class="pt-12 pl-8 hidden md:block">
<%= render(PlausibleWeb.AuthView, "_onboarding_steps.html", current_step: 1) %>
</div>

View File

@ -222,7 +222,7 @@
<div class="my-4 border-b border-gray-300 dark:border-gray-500"></div>
<%= form_for @changeset, "/settings", [class: "max-w-sm"], fn f -> %>
<%= form_for @settings_changeset, "/settings", [class: "max-w-sm"], fn f -> %>
<div class="col-span-4 sm:col-span-2">
<%= label(f, :theme, "Theme Selection",
class: "block text-sm font-medium leading-5 text-gray-700 dark:text-gray-300"
@ -237,11 +237,13 @@
</div>
<div class="max-w-2xl px-8 pt-6 pb-8 mx-auto mt-16 bg-white border-t-2 border-indigo-100 rounded rounded-t-none shadow-md dark:bg-gray-800 dark:border-indigo-500">
<h2 class="text-xl font-black dark:text-gray-100">Account settings</h2>
<h2 id="change-account-name" class="text-xl font-black dark:text-gray-100">
Change account name
</h2>
<div class="my-4 border-b border-gray-300 dark:border-gray-500"></div>
<%= form_for @changeset, "/settings", [class: "max-w-sm"], fn f -> %>
<%= form_for @settings_changeset, "/settings#change-account-name", [class: "max-w-sm"], fn f -> %>
<div class="my-4">
<%= label(f, :name, class: "block text-sm font-medium text-gray-700 dark:text-gray-300") %>
<div class="mt-1">
@ -252,17 +254,61 @@
<%= error_tag(f, :name) %>
</div>
</div>
<%= submit("Save", class: "button mt-4") %>
<% end %>
</div>
<div class="max-w-2xl px-8 pt-6 pb-8 mx-auto mt-16 bg-white border-t-2 border-red-600 rounded rounded-t-none shadow-md dark:bg-gray-800">
<h2 id="change-email-address" class="text-xl font-black dark:text-gray-100">
Change email address
</h2>
<div class="my-4 border-b border-gray-300 dark:border-gray-500"></div>
<%= form_for @email_changeset, "/settings/email#change-email-address", [class: "max-w-sm"], fn f -> %>
<div class="my-4">
<%= label(f, :email, class: "block text-sm font-medium text-gray-700 dark:text-gray-300") %>
<%= label(f, :password, "Account password",
class: "block text-sm font-medium text-gray-700 dark:text-gray-300"
) %>
<div class="mt-1">
<%= password_input(f, :password,
class:
"shadow-sm dark:bg-gray-900 dark:text-gray-300 focus:ring-indigo-500 focus:border-indigo-500 block w-full sm:text-sm border-gray-300 dark:border-gray-500 rounded-md dark:bg-gray-800"
) %>
<%= error_tag(f, :password) %>
</div>
</div>
<div class="my-4">
<%= label(f, :current_email, "Current email",
class: "block text-sm font-medium text-gray-700 dark:text-gray-300"
) %>
<div class="mt-1">
<%= email_input(f, :current_email,
readonly: true,
value: f.data.email,
class:
"shadow-sm dark:bg-gray-900 dark:text-gray-300 focus:ring-indigo-500 focus:border-indigo-500 block w-full sm:text-sm border-gray-300 dark:border-gray-500 rounded-md dark:bg-gray-800 bg-gray-100"
) %>
</div>
</div>
<div class="my-4">
<%= label(f, :email, "New email",
class: "block text-sm font-medium text-gray-700 dark:text-gray-300"
) %>
<div class="mt-1">
<%= email_input(f, :email,
value: f.params["email"],
class:
"shadow-sm dark:bg-gray-900 dark:text-gray-300 focus:ring-indigo-500 focus:border-indigo-500 block w-full sm:text-sm border-gray-300 dark:border-gray-500 rounded-md"
) %>
<%= error_tag(f, :email) %>
</div>
</div>
<%= submit("Save changes", class: "button mt-4") %>
<%= submit("Change my email",
class:
"inline-block mt-4 px-4 py-2 border border-gray-300 dark:border-gray-500 text-sm leading-5 font-medium rounded-md text-red-700 bg-white dark:bg-gray-800 hover:text-red-500 dark:hover:text-red-400 focus:outline-none focus:border-blue-300 focus:ring active:text-red-800 active:bg-gray-50 transition ease-in-out duration-150"
) %>
<% end %>
</div>

View File

@ -52,7 +52,7 @@ seeded_token = Plausible.Plugins.API.Token.generate("seed-token")
{:ok, goal2} = Plausible.Goals.create(site, %{"page_path" => "/register"})
{:ok, goal3} = Plausible.Goals.create(site, %{"page_path" => "/login"})
{:ok, goal4} = Plausible.Goals.create(site, %{"event_name" => "Purchase", "currency" => "USD"})
{:ok, goal5} = Plausible.Goals.create(site, %{"page_path" => Enum.random(long_random_paths)})
{:ok, _goal5} = Plausible.Goals.create(site, %{"page_path" => Enum.random(long_random_paths)})
{:ok, outbound} = Plausible.Goals.create(site, %{"event_name" => "Outbound Link: Click"})
{:ok, _funnel} =

View File

@ -440,7 +440,9 @@ defmodule PlausibleWeb.AuthControllerTest do
test "shows the form", %{conn: conn} do
conn = get(conn, "/settings")
assert html_response(conn, 200) =~ "Account settings"
assert resp = html_response(conn, 200)
assert resp =~ "Change account name"
assert resp =~ "Change email address"
end
test "shows subscription", %{conn: conn, user: user} do
@ -686,6 +688,153 @@ defmodule PlausibleWeb.AuthControllerTest do
end
end
describe "PUT /settings/email" do
setup [:create_user, :log_in]
test "updates email and forces reverification", %{conn: conn, user: user} do
password = "very-long-very-secret-123"
user
|> User.set_password(password)
|> Repo.update!()
assert user.email_verified
conn =
put(conn, "/settings/email", %{
"user" => %{"email" => "new" <> user.email, "password" => password}
})
assert redirected_to(conn, 302) == Routes.auth_path(conn, :activate)
updated_user = Repo.reload!(user)
assert updated_user.email == "new" <> user.email
assert updated_user.previous_email == user.email
refute updated_user.email_verified
assert_delivered_email_matches(%{to: [{_, user_email}], subject: subject})
assert user_email == updated_user.email
assert subject =~ "is your Plausible email verification code"
end
test "renders form with error on no fields filled", %{conn: conn} do
conn = put(conn, "/settings/email", %{"user" => %{}})
assert html_response(conn, 200) =~ "can&#39;t be blank"
end
test "renders form with error on invalid password", %{conn: conn, user: user} do
conn =
put(conn, "/settings/email", %{
"user" => %{"password" => "invalid", "email" => "new" <> user.email}
})
assert html_response(conn, 200) =~ "is invalid"
end
test "renders form with error on already taken email", %{conn: conn, user: user} do
other_user = insert(:user)
password = "very-long-very-secret-123"
user
|> User.set_password(password)
|> Repo.update!()
conn =
put(conn, "/settings/email", %{
"user" => %{"password" => password, "email" => other_user.email}
})
assert html_response(conn, 200) =~ "has already been taken"
end
test "renders form with error when email is identical with the current one", %{
conn: conn,
user: user
} do
password = "very-long-very-secret-123"
user
|> User.set_password(password)
|> Repo.update!()
conn =
put(conn, "/settings/email", %{
"user" => %{"password" => password, "email" => user.email}
})
assert html_response(conn, 200) =~ "can&#39;t be the same"
end
end
describe "POST /settings/email/cancel" do
setup [:create_user, :log_in]
test "cancels email reverification in progress", %{conn: conn, user: user} do
user =
user
|> Ecto.Changeset.change(
email_verified: false,
email: "new" <> user.email,
previous_email: user.email
)
|> Repo.update!()
conn = post(conn, "/settings/email/cancel")
assert redirected_to(conn, 302) ==
Routes.auth_path(conn, :user_settings) <> "#change-email-address"
updated_user = Repo.reload!(user)
assert updated_user.email_verified
assert updated_user.email == user.previous_email
refute updated_user.previous_email
end
test "fails to cancel reverification when previous email is already retaken", %{
conn: conn,
user: user
} do
user =
user
|> Ecto.Changeset.change(
email_verified: false,
email: "new" <> user.email,
previous_email: user.email
)
|> Repo.update!()
_other_user = insert(:user, email: user.previous_email)
conn = post(conn, "/settings/email/cancel")
assert redirected_to(conn, 302) == Routes.auth_path(conn, :activate_form)
assert Phoenix.Flash.get(conn.assigns.flash, :error) =~
"Could not cancel email update"
end
test "crashes when previous email is empty on cancel (should not happen)", %{
conn: conn,
user: user
} do
user
|> Ecto.Changeset.change(
email_verified: false,
email: "new" <> user.email,
previous_email: nil
)
|> Repo.update!()
assert_raise RuntimeError, ~r/Previous email is empty for user/, fn ->
post(conn, "/settings/email/cancel")
end
end
end
describe "DELETE /me" do
setup [:create_user, :log_in, :create_new_site]
use Plausible.Repo