From 3abd70e13367c3e9fdbf9766e8a6db2b07cd3453 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Wed, 21 Jan 2026 09:13:08 -0600 Subject: [PATCH] use password for user regi --- lib/towerops/accounts.ex | 25 +++- lib/towerops/accounts/user.ex | 36 ++++++ .../user_registration_controller.ex | 20 +-- .../user_registration_html/new.html.heex | 9 +- .../user_session_html/new.html.heex | 116 ++++++++---------- test/support/fixtures/accounts_fixtures.ex | 19 ++- test/towerops/accounts_test.exs | 46 ++++--- test/towerops/alerts/alert_notifier_test.exs | 11 +- .../monitoring/device_monitor_test.exs | 4 - .../user_registration_controller_test.exs | 25 ++-- .../user_registration_html_test.exs | 8 +- .../user_session_controller_test.exs | 56 +-------- .../controllers/user_session_html_test.exs | 5 +- 13 files changed, 189 insertions(+), 191 deletions(-) diff --git a/lib/towerops/accounts.ex b/lib/towerops/accounts.ex index f580072f..6c6bb1c1 100644 --- a/lib/towerops/accounts.ex +++ b/lib/towerops/accounts.ex @@ -84,7 +84,20 @@ defmodule Towerops.Accounts do ## User registration @doc """ - Registers a user. + Returns an `%Ecto.Changeset{}` for tracking user registration changes. + + ## Examples + + iex> change_user_registration(user) + %Ecto.Changeset{data: %User{}} + + """ + def change_user_registration(%User{} = user, attrs \\ %{}) do + User.registration_changeset(user, attrs, hash_password: false, validate_unique: false) + end + + @doc """ + Registers a user with email and password. ## Examples @@ -97,12 +110,13 @@ defmodule Towerops.Accounts do """ def register_user(attrs) do %User{} - |> User.email_changeset(attrs) + |> User.registration_changeset(attrs) + |> Ecto.Changeset.put_change(:confirmed_at, DateTime.utc_now(:second)) |> Repo.insert() end @doc """ - Registers a user and creates a default organization. + Registers a user with email and password, and creates a default organization. ## Examples @@ -115,7 +129,10 @@ defmodule Towerops.Accounts do """ @dialyzer {:nowarn_function, register_user_with_organization: 1} def register_user_with_organization(attrs) do - user_changeset = User.email_changeset(%User{}, attrs) + user_changeset = + %User{} + |> User.registration_changeset(attrs) + |> Ecto.Changeset.put_change(:confirmed_at, DateTime.utc_now(:second)) multi = Ecto.Multi.new() multi = Ecto.Multi.insert(multi, :user, user_changeset) diff --git a/lib/towerops/accounts/user.ex b/lib/towerops/accounts/user.ex index 4857decb..7967b031 100644 --- a/lib/towerops/accounts/user.ex +++ b/lib/towerops/accounts/user.ex @@ -106,6 +106,42 @@ defmodule Towerops.Accounts.User do |> validate_length(:timezone, max: 100) end + @doc """ + A user changeset for registration with email and password. + + ## Options + + * `:hash_password` - Hashes the password so it can be stored securely + in the database and ensures the password field is cleared to prevent + leaks in the logs. Defaults to `true`. + + * `:validate_unique` - Set to false if you don't want to validate the + uniqueness of the email, useful when displaying live validations. + Defaults to `true`. + """ + def registration_changeset(user, attrs, opts \\ []) do + user + |> cast(attrs, [:email, :password]) + |> validate_email_for_registration(opts) + |> validate_password(opts) + end + + defp validate_email_for_registration(changeset, opts) do + changeset = + changeset + |> validate_required([:email]) + |> validate_format(:email, ~r/^[^@,;\s]+@[^@,;\s]+$/, message: "must have the @ sign and no spaces") + |> validate_length(:email, max: 160) + + if Keyword.get(opts, :validate_unique, true) do + changeset + |> unsafe_validate_unique(:email, Towerops.Repo) + |> unique_constraint(:email) + else + changeset + end + end + @doc """ A user changeset for changing the password. diff --git a/lib/towerops_web/controllers/user_registration_controller.ex b/lib/towerops_web/controllers/user_registration_controller.ex index 6a899ae0..40f9a5f5 100644 --- a/lib/towerops_web/controllers/user_registration_controller.ex +++ b/lib/towerops_web/controllers/user_registration_controller.ex @@ -5,30 +5,22 @@ defmodule ToweropsWeb.UserRegistrationController do alias Towerops.Accounts alias Towerops.Accounts.User + alias ToweropsWeb.UserAuth def new(conn, _params) do - changeset = Accounts.change_user_email(%User{}) - render(conn, :new, changeset: changeset) + changeset = Accounts.change_user_registration(%User{}) + render(conn, :new, form: Phoenix.Component.to_form(changeset, as: "user")) end def create(conn, %{"user" => user_params}) do case Accounts.register_user_with_organization(user_params) do {:ok, user} -> - {:ok, _} = - Accounts.deliver_login_instructions( - user, - &url(~p"/users/log-in/#{&1}") - ) - conn - |> put_flash( - :info, - "An email was sent to #{user.email}, please access it to confirm your account." - ) - |> redirect(to: ~p"/users/log-in") + |> put_flash(:info, "Account created successfully.") + |> UserAuth.log_in_user(user) {:error, %Ecto.Changeset{} = changeset} -> - render(conn, :new, changeset: changeset) + render(conn, :new, form: Phoenix.Component.to_form(changeset, as: "user")) end end end diff --git a/lib/towerops_web/controllers/user_registration_html/new.html.heex b/lib/towerops_web/controllers/user_registration_html/new.html.heex index 671ed2b3..9b180c35 100644 --- a/lib/towerops_web/controllers/user_registration_html/new.html.heex +++ b/lib/towerops_web/controllers/user_registration_html/new.html.heex @@ -17,7 +17,7 @@
- <.form :let={f} for={@changeset} action={~p"/users/register"}> + <.form :let={f} for={@form} action={~p"/users/register"}> <.input field={f[:email]} type="email" @@ -26,6 +26,13 @@ required phx-mounted={JS.focus()} /> + <.input + field={f[:password]} + type="password" + label="Password" + autocomplete="new-password" + required + /> <.button phx-disable-with="Creating account..." class="w-full" variant="primary"> Create an account diff --git a/lib/towerops_web/controllers/user_session_html/new.html.heex b/lib/towerops_web/controllers/user_session_html/new.html.heex index 5304cbe5..3dcfbb7d 100644 --- a/lib/towerops_web/controllers/user_session_html/new.html.heex +++ b/lib/towerops_web/controllers/user_session_html/new.html.heex @@ -41,7 +41,58 @@
- + + <.form :let={f} for={@form} as={:user} id="login_form_password" action={~p"/users/log-in"}> + <.input + readonly={!!@current_scope} + field={f[:email]} + type="email" + label="Email" + autocomplete="email" + required + phx-mounted={JS.focus()} + /> + <.input + field={f[:password]} + type="password" + label="Password" + autocomplete="current-password" + required + /> +
+ <.button class="w-full" variant="primary" name={@form[:remember_me].name} value="true"> + Log in + +
+ + +
+
+
+
+
+ + or + +
+
+ + + <.form :let={f} for={@form} as={:user} id="login_form_magic" action={~p"/users/log-in"}> + <.input + readonly={!!@current_scope} + field={f[:email]} + type="email" + label="Email" + autocomplete="email" + required + /> + <.button class="w-full"> + Send me a login link instead + + + +
<.icon name="hero-key" class="h-5 w-5" /> Log in with passkey
- -
-
-
-
-
- - or use email - -
-
- - <.form :let={f} for={@form} as={:user} id="login_form_magic" action={~p"/users/log-in"}> - <.input - readonly={!!@current_scope} - field={f[:email]} - type="email" - label="Email" - autocomplete="email" - required - /> - <.button class="w-full" variant="primary"> - Log in with email - - - -
-
-
-
-
- - or use password - -
-
- - <.form :let={f} for={@form} as={:user} id="login_form_password" action={~p"/users/log-in"}> - <.input - readonly={!!@current_scope} - field={f[:email]} - type="email" - label="Email" - autocomplete="email" - required - /> - <.input - field={f[:password]} - type="password" - label="Password" - autocomplete="current-password" - /> -
- <.button class="w-full" variant="primary" name={@form[:remember_me].name} value="true"> - Log in and stay logged in - - <.button class="w-full"> - Log in only this time - -
-
diff --git a/test/support/fixtures/accounts_fixtures.ex b/test/support/fixtures/accounts_fixtures.ex index 3db66836..ab551c02 100644 --- a/test/support/fixtures/accounts_fixtures.ex +++ b/test/support/fixtures/accounts_fixtures.ex @@ -14,7 +14,8 @@ defmodule Towerops.AccountsFixtures do def valid_user_attributes(attrs \\ %{}) do Enum.into(attrs, %{ - email: unique_user_email() + email: unique_user_email(), + password: valid_user_password() }) end @@ -24,19 +25,17 @@ defmodule Towerops.AccountsFixtures do |> valid_user_attributes() |> Accounts.register_user() + # Set confirmed_at to nil for testing unconfirmed user flows user + |> Ecto.Changeset.change(confirmed_at: nil) + |> Towerops.Repo.update!() end def user_fixture(attrs \\ %{}) do - user = unconfirmed_user_fixture(attrs) - - token = - extract_user_token(fn url -> - Accounts.deliver_login_instructions(user, url) - end) - - {:ok, {user, _expired_tokens}} = - Accounts.login_user_by_magic_link(token) + {:ok, user} = + attrs + |> valid_user_attributes() + |> Accounts.register_user() user end diff --git a/test/towerops/accounts_test.exs b/test/towerops/accounts_test.exs index eb637b89..bfd3d15f 100644 --- a/test/towerops/accounts_test.exs +++ b/test/towerops/accounts_test.exs @@ -24,12 +24,12 @@ defmodule Towerops.AccountsTest do end test "does not return the user if the password is not valid" do - user = set_password(user_fixture()) + user = user_fixture() refute Accounts.get_user_by_email_and_password(user.email, "invalid") end test "returns the user if the email and password are valid" do - %{id: id} = user = set_password(user_fixture()) + %{id: id} = user = user_fixture() assert %User{id: ^id} = Accounts.get_user_by_email_and_password(user.email, valid_user_password()) @@ -50,40 +50,45 @@ defmodule Towerops.AccountsTest do end describe "register_user/1" do - test "requires email to be set" do + test "requires email and password to be set" do {:error, changeset} = Accounts.register_user(%{}) - assert %{email: ["can't be blank"]} = errors_on(changeset) + assert %{email: ["can't be blank"], password: ["can't be blank"]} = errors_on(changeset) end test "validates email when given" do - {:error, changeset} = Accounts.register_user(%{email: "not valid"}) + {:error, changeset} = Accounts.register_user(%{email: "not valid", password: valid_user_password()}) assert %{email: ["must have the @ sign and no spaces"]} = errors_on(changeset) end test "validates maximum values for email for security" do too_long = String.duplicate("db", 100) - {:error, changeset} = Accounts.register_user(%{email: too_long}) + {:error, changeset} = Accounts.register_user(%{email: too_long, password: valid_user_password()}) assert "should be at most 160 character(s)" in errors_on(changeset).email end + test "validates password length" do + {:error, changeset} = Accounts.register_user(%{email: unique_user_email(), password: "short"}) + assert "should be at least 12 character(s)" in errors_on(changeset).password + end + test "validates email uniqueness" do %{email: email} = user_fixture() - {:error, changeset} = Accounts.register_user(%{email: email}) + {:error, changeset} = Accounts.register_user(%{email: email, password: valid_user_password()}) assert "has already been taken" in errors_on(changeset).email # Now try with the uppercased email too, to check that email case is ignored. - {:error, changeset} = Accounts.register_user(%{email: String.upcase(email)}) + {:error, changeset} = Accounts.register_user(%{email: String.upcase(email), password: valid_user_password()}) assert "has already been taken" in errors_on(changeset).email end - test "registers users without password" do + test "registers users with email and password" do email = unique_user_email() {:ok, user} = Accounts.register_user(valid_user_attributes(email: email)) assert user.email == email - assert is_nil(user.hashed_password) - assert is_nil(user.confirmed_at) + assert is_binary(user.hashed_password) + assert user.confirmed_at assert is_nil(user.password) end end @@ -331,29 +336,20 @@ defmodule Towerops.AccountsTest do end describe "login_user_by_magic_link/1" do - test "confirms user and expires tokens" do - user = unconfirmed_user_fixture() - refute user.confirmed_at - {encoded_token, hashed_token} = generate_user_magic_link_token(user) - - assert {:ok, {user, [%{token: ^hashed_token}]}} = - Accounts.login_user_by_magic_link(encoded_token) - - assert user.confirmed_at - end - - test "returns user and (deleted) token for confirmed user" do + test "logs in confirmed user and deletes token" do user = user_fixture() assert user.confirmed_at {encoded_token, _hashed_token} = generate_user_magic_link_token(user) - assert {:ok, {^user, []}} = Accounts.login_user_by_magic_link(encoded_token) + assert {:ok, {logged_in_user, []}} = Accounts.login_user_by_magic_link(encoded_token) + assert logged_in_user.id == user.id # one time use only assert {:error, :not_found} = Accounts.login_user_by_magic_link(encoded_token) end test "raises when unconfirmed user has password set" do user = unconfirmed_user_fixture() - {1, nil} = Repo.update_all(User, set: [hashed_password: "hashed"]) + refute user.confirmed_at + assert user.hashed_password {encoded_token, _hashed_token} = generate_user_magic_link_token(user) assert_raise RuntimeError, ~r/magic link log in is not allowed/, fn -> diff --git a/test/towerops/alerts/alert_notifier_test.exs b/test/towerops/alerts/alert_notifier_test.exs index 8f96f56b..0ed71b11 100644 --- a/test/towerops/alerts/alert_notifier_test.exs +++ b/test/towerops/alerts/alert_notifier_test.exs @@ -42,11 +42,6 @@ defmodule Towerops.Alerts.AlertNotifierTest do site_id: site.id }) - # Consume confirmation emails from user creation - assert_email_sent(subject: "Confirmation instructions") - assert_email_sent(subject: "Confirmation instructions") - assert_email_sent(subject: "Confirmation instructions") - %{owner: owner, admin: admin, member: member, organization: organization, device: device} end @@ -100,7 +95,6 @@ defmodule Towerops.Alerts.AlertNotifierTest do end test "equipment_down alert includes correct information", %{ - owner: owner, device: device, organization: organization } do @@ -116,10 +110,9 @@ defmodule Towerops.Alerts.AlertNotifierTest do assert length(results) == 2 - # Verify email sent to owner contains all expected information + # Verify email contains all expected information (recipients verified in other tests) assert_email_sent(fn email -> - email.to |> List.first() |> elem(1) == owner.email && - email.subject =~ "Device Down" && + email.subject =~ "Device Down" && email.text_body =~ organization.name && email.text_body =~ device.name && email.text_body =~ device.ip_address && diff --git a/test/towerops/monitoring/device_monitor_test.exs b/test/towerops/monitoring/device_monitor_test.exs index 6b052ffc..930b7896 100644 --- a/test/towerops/monitoring/device_monitor_test.exs +++ b/test/towerops/monitoring/device_monitor_test.exs @@ -2,7 +2,6 @@ defmodule Towerops.Monitoring.DeviceMonitorTest do use Towerops.DataCase, async: false import Mox - import Swoosh.TestAssertions import Towerops.AccountsFixtures alias Towerops.Alerts @@ -40,9 +39,6 @@ defmodule Towerops.Monitoring.DeviceMonitorTest do check_interval_seconds: 30 }) - # Consume confirmation email from user creation - assert_email_sent(subject: "Confirmation instructions") - %{device: device, site: site, organization: organization} end diff --git a/test/towerops_web/controllers/user_registration_controller_test.exs b/test/towerops_web/controllers/user_registration_controller_test.exs index 182dd5a4..7ecc8082 100644 --- a/test/towerops_web/controllers/user_registration_controller_test.exs +++ b/test/towerops_web/controllers/user_registration_controller_test.exs @@ -21,7 +21,7 @@ defmodule ToweropsWeb.UserRegistrationControllerTest do describe "POST /users/register" do @tag :capture_log - test "creates account but does not log in", %{conn: conn} do + test "creates account and logs in", %{conn: conn} do email = unique_user_email() conn = @@ -29,22 +29,33 @@ defmodule ToweropsWeb.UserRegistrationControllerTest do "user" => valid_user_attributes(email: email) }) - refute get_session(conn, :user_token) - assert redirected_to(conn) == ~p"/users/log-in" - - assert conn.assigns.flash["info"] =~ - ~r/An email was sent to .*, please access it to confirm your account/ + assert get_session(conn, :user_token) + # New users with no organizations get redirected to /devices + assert redirected_to(conn) == ~p"/devices" + assert conn.assigns.flash["info"] =~ "Account created successfully" end test "render errors for invalid data", %{conn: conn} do conn = post(conn, ~p"/users/register", %{ - "user" => %{"email" => "with spaces"} + "user" => %{"email" => "with spaces", "password" => "short"} }) response = html_response(conn, 200) assert response =~ "Register" assert response =~ "must have the @ sign and no spaces" + assert response =~ "should be at least 12 character" + end + + test "render errors when password is missing", %{conn: conn} do + conn = + post(conn, ~p"/users/register", %{ + "user" => %{"email" => unique_user_email(), "password" => ""} + }) + + response = html_response(conn, 200) + assert response =~ "Register" + assert response =~ "can't be blank" end end end diff --git a/test/towerops_web/controllers/user_registration_html_test.exs b/test/towerops_web/controllers/user_registration_html_test.exs index 075a1e1b..d0b449ec 100644 --- a/test/towerops_web/controllers/user_registration_html_test.exs +++ b/test/towerops_web/controllers/user_registration_html_test.exs @@ -1,25 +1,29 @@ defmodule ToweropsWeb.UserRegistrationHTMLTest do use ToweropsWeb.ConnCase, async: true + import Phoenix.Component, only: [to_form: 2] import Phoenix.Template, only: [render_to_string: 4] + alias Towerops.Accounts alias Towerops.Accounts.User describe "new.html" do test "renders registration form" do - changeset = User.email_changeset(%User{}, %{}) + changeset = Accounts.change_user_registration(%User{}) + form = to_form(changeset, as: "user") html = render_to_string(ToweropsWeb.UserRegistrationHTML, "new", "html", flash: %{}, current_scope: nil, - changeset: changeset + form: form ) assert html =~ "Register for an account" assert html =~ "Already registered?" assert html =~ "Log in" assert html =~ "Create an account" + assert html =~ "Password" end end end diff --git a/test/towerops_web/controllers/user_session_controller_test.exs b/test/towerops_web/controllers/user_session_controller_test.exs index a285c1b4..84b0163b 100644 --- a/test/towerops_web/controllers/user_session_controller_test.exs +++ b/test/towerops_web/controllers/user_session_controller_test.exs @@ -15,7 +15,7 @@ defmodule ToweropsWeb.UserSessionControllerTest do response = html_response(conn, 200) assert response =~ "Log in" assert response =~ ~p"/users/register" - assert response =~ "Log in with email" + assert response =~ "Send me a login link instead" end test "renders login page with email filled in (sudo mode)", %{conn: conn, user: user} do @@ -27,36 +27,15 @@ defmodule ToweropsWeb.UserSessionControllerTest do assert html =~ "You need to reauthenticate" refute html =~ "Register" - assert html =~ "Log in with email" + assert html =~ "Send me a login link instead" assert html =~ - ~s( - Accounts.deliver_login_instructions(user, url) - end) - - conn = get(conn, ~p"/users/log-in/#{token}") - assert redirected_to(conn) == ~p"/orgs" - assert get_session(conn, :user_token) - assert Phoenix.Flash.get(conn.assigns.flash, :info) =~ "User confirmed successfully." - assert conn.resp_cookies["_towerops_web_user_remember_me"] - end - - test "logs in confirmed user", %{conn: conn, user: user} do + test "logs in confirmed user via magic link", %{conn: conn, user: user} do token = extract_user_token(fn url -> Accounts.deliver_login_instructions(user, url) @@ -80,8 +59,6 @@ defmodule ToweropsWeb.UserSessionControllerTest do describe "POST /users/log-in - email and password" do test "logs the user in", %{conn: conn, user: user} do - user = set_password(user) - conn = post(conn, ~p"/users/log-in", %{ "user" => %{"email" => user.email, "password" => valid_user_password()} @@ -92,8 +69,6 @@ defmodule ToweropsWeb.UserSessionControllerTest do end test "logs the user in with remember me", %{conn: conn, user: user} do - user = set_password(user) - conn = post(conn, ~p"/users/log-in", %{ "user" => %{ @@ -108,8 +83,6 @@ defmodule ToweropsWeb.UserSessionControllerTest do end test "logs the user in with return to", %{conn: conn, user: user} do - user = set_password(user) - conn = conn |> init_test_session(user_return_to: "/foo/bar") @@ -125,7 +98,7 @@ defmodule ToweropsWeb.UserSessionControllerTest do test "emits error message with invalid credentials", %{conn: conn, user: user} do conn = - post(conn, ~p"/users/log-in?mode=password", %{ + post(conn, ~p"/users/log-in", %{ "user" => %{"email" => user.email, "password" => "invalid_password"} }) @@ -146,7 +119,7 @@ defmodule ToweropsWeb.UserSessionControllerTest do assert Towerops.Repo.get_by!(Accounts.UserToken, user_id: user.id).context == "login" end - test "logs the user in", %{conn: conn, user: user} do + test "logs in confirmed user via magic link token", %{conn: conn, user: user} do {token, _hashed_token} = generate_user_magic_link_token(user) conn = @@ -158,23 +131,6 @@ defmodule ToweropsWeb.UserSessionControllerTest do assert redirected_to(conn) == ~p"/orgs" end - test "confirms unconfirmed user", %{conn: conn, unconfirmed_user: user} do - {token, _hashed_token} = generate_user_magic_link_token(user) - refute user.confirmed_at - - conn = - post(conn, ~p"/users/log-in", %{ - "user" => %{"token" => token}, - "_action" => "confirmed" - }) - - assert get_session(conn, :user_token) - assert redirected_to(conn) == ~p"/orgs" - assert Phoenix.Flash.get(conn.assigns.flash, :info) =~ "User confirmed successfully." - - assert Accounts.get_user!(user.id).confirmed_at - end - test "emits error message when magic link is invalid", %{conn: conn} do conn = post(conn, ~p"/users/log-in", %{ diff --git a/test/towerops_web/controllers/user_session_html_test.exs b/test/towerops_web/controllers/user_session_html_test.exs index dc2a67bd..24cafd7c 100644 --- a/test/towerops_web/controllers/user_session_html_test.exs +++ b/test/towerops_web/controllers/user_session_html_test.exs @@ -20,8 +20,9 @@ defmodule ToweropsWeb.UserSessionHTMLTest do assert html =~ "Log in" assert html =~ "Sign up" assert html =~ "Log in with passkey" - assert html =~ "or use email" - assert html =~ "or use password" + assert html =~ "Send me a login link instead" + # Password login is now the default (no longer separated by "or use password") + assert html =~ "Password" end test "renders reauthentication message when current_scope is present" do