From 3b0487fd75c3a621303d9847343f3a36851e09b4 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Wed, 11 Mar 2026 10:37:03 -0500 Subject: [PATCH] add 100% test coverage for Towerops.Accounts - add 23 new ExUnit tests covering all previously uncovered code paths - fix touch_changeset bug: DateTime.utc_now() -> DateTime.utc_now(:second) to avoid microsecond errors with :utc_datetime fields - add defensive coverage tests using PostgreSQL triggers to exercise Multi transaction catch-all clauses and partial insert_all paths - coverage: 88.67% -> 100.00% (169 -> 192 tests) --- lib/towerops/accounts/browser_session.ex | 2 +- .../accounts_defensive_coverage_test.exs | 116 +++++ test/towerops/accounts_test.exs | 397 ++++++++++++++++++ 3 files changed, 514 insertions(+), 1 deletion(-) create mode 100644 test/towerops/accounts_defensive_coverage_test.exs diff --git a/lib/towerops/accounts/browser_session.ex b/lib/towerops/accounts/browser_session.ex index 38047d3d..40419f5f 100644 --- a/lib/towerops/accounts/browser_session.ex +++ b/lib/towerops/accounts/browser_session.ex @@ -122,7 +122,7 @@ defmodule Towerops.Accounts.BrowserSession do to keep track of when the session was last used. """ def touch_changeset(browser_session) do - change(browser_session, %{last_activity_at: DateTime.utc_now()}) + change(browser_session, %{last_activity_at: DateTime.utc_now(:second)}) end @doc """ diff --git a/test/towerops/accounts_defensive_coverage_test.exs b/test/towerops/accounts_defensive_coverage_test.exs new file mode 100644 index 00000000..bce2f971 --- /dev/null +++ b/test/towerops/accounts_defensive_coverage_test.exs @@ -0,0 +1,116 @@ +defmodule Towerops.AccountsDefensiveCoverageTest do + @moduledoc """ + Tests for defensive code paths in Towerops.Accounts that require + database-level manipulation (triggers/constraints) to exercise. + + These tests use raw SQL to create temporary triggers that are + rolled back by the Ecto sandbox after each test. + """ + use Towerops.DataCase + + import Towerops.AccountsFixtures + + alias Towerops.Accounts + + describe "register_user/1 Multi catch-all (L136)" do + test "returns error when consent grant fails due to FK violation" do + # Create a BEFORE INSERT trigger on user_consents that deletes the user, + # causing the FK constraint to fail. The FK is declared in the changeset + # so Ecto catches it and returns {:error, changeset}. + Repo.query!(""" + CREATE OR REPLACE FUNCTION test_delete_user_on_consent_insert() + RETURNS TRIGGER AS $$ + BEGIN + DELETE FROM users WHERE id = NEW.user_id; + RETURN NEW; + END; + $$ LANGUAGE plpgsql; + """) + + Repo.query!(""" + CREATE TRIGGER test_consent_fk_trigger + BEFORE INSERT ON user_consents + FOR EACH ROW + EXECUTE FUNCTION test_delete_user_on_consent_insert(); + """) + + attrs = %{ + "email" => unique_user_email(), + "password" => valid_user_password(), + "privacy_policy_consent" => "true", + "terms_of_service_consent" => "true" + } + + assert {:error, changeset} = Accounts.register_user(attrs) + assert %Ecto.Changeset{} = changeset + end + end + + describe "register_user_with_organization/1 Multi catch-all (L177)" do + test "returns error when consent grant fails due to FK violation" do + Repo.query!(""" + CREATE OR REPLACE FUNCTION test_delete_user_on_consent_insert() + RETURNS TRIGGER AS $$ + BEGIN + DELETE FROM users WHERE id = NEW.user_id; + RETURN NEW; + END; + $$ LANGUAGE plpgsql; + """) + + Repo.query!(""" + CREATE TRIGGER test_consent_fk_trigger + BEFORE INSERT ON user_consents + FOR EACH ROW + EXECUTE FUNCTION test_delete_user_on_consent_insert(); + """) + + attrs = %{ + "email" => unique_user_email(), + "password" => valid_user_password(), + "privacy_policy_consent" => "true", + "terms_of_service_consent" => "true", + "organization_name" => "Test Org" + } + + assert {:error, changeset} = Accounts.register_user_with_organization(attrs) + assert %Ecto.Changeset{} = changeset + end + end + + describe "generate_recovery_codes/1 partial insert (L514)" do + test "returns error when insert_all inserts fewer than 12 codes" do + user = user_fixture() + + # Create a BEFORE INSERT trigger that skips half the recovery code rows + # by returning NULL for even-numbered rows + Repo.query!(""" + CREATE SEQUENCE IF NOT EXISTS test_recovery_counter START 1; + """) + + Repo.query!(""" + CREATE OR REPLACE FUNCTION test_skip_recovery_codes() + RETURNS TRIGGER AS $$ + DECLARE + val int; + BEGIN + val := nextval('test_recovery_counter'); + IF val % 2 = 0 THEN + RETURN NULL; + END IF; + RETURN NEW; + END; + $$ LANGUAGE plpgsql; + """) + + Repo.query!(""" + CREATE TRIGGER test_skip_recovery_trigger + BEFORE INSERT ON user_recovery_codes + FOR EACH ROW + EXECUTE FUNCTION test_skip_recovery_codes(); + """) + + assert {:error, :generation_failed} = Accounts.generate_recovery_codes(user.id) + end + end +end diff --git a/test/towerops/accounts_test.exs b/test/towerops/accounts_test.exs index ba74f574..cc3f8987 100644 --- a/test/towerops/accounts_test.exs +++ b/test/towerops/accounts_test.exs @@ -5,7 +5,10 @@ defmodule Towerops.AccountsTest do alias Towerops.Accounts alias Towerops.Accounts.User + alias Towerops.Accounts.UserRecoveryCode alias Towerops.Accounts.UserToken + alias Towerops.GeoIP.Block + alias Towerops.GeoIP.Location describe "get_user_by_email/1" do test "does not return the user if the email does not exist" do @@ -1889,6 +1892,83 @@ defmodule Towerops.AccountsTest do end end + describe "deliver_user_confirmation_instructions/2" do + test "sends token through notification" do + user = unconfirmed_user_fixture() + + token = + extract_user_token(fn url -> + Accounts.deliver_user_confirmation_instructions(user, url) + end) + + {:ok, token} = Base.url_decode64(token, padding: false) + assert user_token = Repo.get_by(UserToken, token: :crypto.hash(:sha256, token)) + assert user_token.user_id == user.id + assert user_token.sent_to == user.email + assert user_token.context == "confirm" + end + end + + describe "confirm_user/1" do + test "confirms user with valid token" do + user = unconfirmed_user_fixture() + refute user.confirmed_at + + token = + extract_user_token(fn url -> + Accounts.deliver_user_confirmation_instructions(user, url) + end) + + assert {:ok, confirmed_user} = Accounts.confirm_user(token) + assert confirmed_user.confirmed_at + assert confirmed_user.id == user.id + end + + test "deletes confirmation tokens after confirming" do + user = unconfirmed_user_fixture() + + token = + extract_user_token(fn url -> + Accounts.deliver_user_confirmation_instructions(user, url) + end) + + assert {:ok, _} = Accounts.confirm_user(token) + # Token should be consumed + refute Repo.get_by(UserToken, user_id: user.id, context: "confirm") + end + + test "returns error for invalid token" do + assert :error = Accounts.confirm_user("invalid-token") + end + + test "returns error for expired token" do + user = unconfirmed_user_fixture() + + token = + extract_user_token(fn url -> + Accounts.deliver_user_confirmation_instructions(user, url) + end) + + {1, nil} = Repo.update_all(UserToken, set: [inserted_at: ~N[2020-01-01 00:00:00]]) + assert :error = Accounts.confirm_user(token) + end + end + + describe "register_user_with_organization/1 error paths" do + test "returns error when organization creation fails" do + attrs = %{ + "email" => unique_user_email(), + "password" => valid_user_password(), + "privacy_policy_consent" => "true", + "terms_of_service_consent" => "true", + "organization_name" => "A" + } + + assert {:error, changeset} = Accounts.register_user_with_organization(attrs) + assert "should be at least 2 character(s)" in errors_on(changeset).name + end + end + describe "register_user/1 with consent params" do test "grants privacy_policy consent when checkbox is true" do email = unique_user_email() @@ -1941,5 +2021,322 @@ defmodule Towerops.AccountsTest do assert Accounts.has_consent?(user.id, "privacy_policy") assert Accounts.has_consent?(user.id, "terms_of_service") end + + test "grants consent with atom key params" do + email = unique_user_email() + + {:ok, user} = + Accounts.register_user(%{ + email: email, + password: valid_user_password(), + privacy_policy_consent: true, + terms_of_service_consent: true + }) + + consents = Accounts.list_user_consents(user.id) + assert length(consents) == 2 + end + end + + describe "login_user_by_magic_link/1 additional cases" do + test "confirms unconfirmed user without password and logs them in" do + # Create user, set confirmed_at and hashed_password to nil + user = unconfirmed_user_fixture() + + user = + user + |> Ecto.Changeset.change(hashed_password: nil) + |> Repo.update!() + + refute user.confirmed_at + refute user.hashed_password + + {encoded_token, _hashed_token} = generate_user_magic_link_token(user) + + assert {:ok, {logged_in_user, expired_tokens}} = + Accounts.login_user_by_magic_link(encoded_token) + + assert logged_in_user.id == user.id + assert logged_in_user.confirmed_at + assert is_list(expired_tokens) + end + + test "returns error for malformed token that fails verification" do + assert {:error, :not_found} = Accounts.login_user_by_magic_link("not-valid-base64!!!") + end + end + + describe "verify_user_mfa/2 without TOTP" do + test "returns totp_not_enabled when user has no TOTP" do + user = user_fixture(enable_totp: false) + refute Accounts.totp_enabled?(user) + + assert {:error, :totp_not_enabled} = Accounts.verify_user_mfa(user, "123456") + end + end + + describe "verify_totp/2 failure logging" do + test "logs debug info when TOTP verification fails" do + import ExUnit.CaptureLog + + secret = Accounts.generate_totp_secret() + + # Temporarily enable info logging to exercise the failure log path + original_level = Logger.level() + Logger.configure(level: :info) + + log = + capture_log(fn -> + refute Accounts.verify_totp(secret, "000000") + end) + + Logger.configure(level: original_level) + + assert log =~ "TOTP verification failed" + assert log =~ "provided=000000" + end + end + + describe "verify_totp/2 with base32 secret" do + test "works with base32-encoded secret" do + raw_secret = Accounts.generate_totp_secret() + base32_secret = Base.encode32(raw_secret, case: :upper, padding: false) + code = NimbleTOTP.verification_code(raw_secret) + + assert Accounts.verify_totp(base32_secret, code) + end + end + + describe "get_user_token_id_by_value/1 with valid token" do + test "returns token id for valid session token" do + user = user_fixture() + # get_user_token_id_by_value hashes the input with SHA256 before lookup + # So we insert a token whose stored value IS the SHA256 hash of a known value + known_value = "test_session_token_value" + hashed_value = :crypto.hash(:sha256, known_value) + + user_token = + Repo.insert!(%UserToken{ + token: hashed_value, + context: "session", + user_id: user.id, + authenticated_at: DateTime.utc_now(:second) + }) + + assert Accounts.get_user_token_id_by_value(known_value) == user_token.id + end + end + + describe "get_browser_session_by_token_value/1 with valid token" do + test "returns session for valid token value" do + user = user_fixture() + # get_browser_session_by_token_value hashes the input with SHA256 before lookup + known_value = "test_browser_session_token" + hashed_value = :crypto.hash(:sha256, known_value) + + user_token = + Repo.insert!(%UserToken{ + token: hashed_value, + context: "session", + user_id: user.id, + authenticated_at: DateTime.utc_now(:second) + }) + + {:ok, session} = + Accounts.create_browser_session(%{ + user_id: user.id, + user_token_id: user_token.id, + ip_address: "10.0.0.1", + user_agent: "TestBrowser/1.0", + last_activity_at: DateTime.utc_now(:second), + expires_at: DateTime.add(DateTime.utc_now(:second), 14, :day) + }) + + found = Accounts.get_browser_session_by_token_value(known_value) + assert found.id == session.id + end + end + + describe "touch_browser_session/1 via function" do + test "updates last_activity_at timestamp" do + user = user_fixture() + {_token, user_token} = Accounts.generate_user_session_token_with_record(user) + past = DateTime.add(DateTime.utc_now(:second), -3600, :second) + + {:ok, session} = + Accounts.create_browser_session(%{ + user_id: user.id, + user_token_id: user_token.id, + ip_address: "10.0.0.1", + user_agent: "TestBrowser/1.0", + last_activity_at: past, + expires_at: DateTime.add(DateTime.utc_now(:second), 14, :day) + }) + + assert {:ok, updated} = Accounts.touch_browser_session(session) + assert DateTime.after?(updated.last_activity_at, past) + end + end + + describe "record_login_attempt/1 edge cases" do + test "skips GeoIP enrichment when ip_address is nil" do + attrs = %{ + email: "test@example.com", + success: true, + method: "password", + ip_address: nil + } + + # ip_address is required, so this returns a changeset error + # but the nil ip_address path in record_login_attempt is exercised + assert {:error, changeset} = Accounts.record_login_attempt(attrs) + assert "can't be blank" in errors_on(changeset).ip_address + end + + test "enriches with GeoIP data when available" do + user = user_fixture() + # Insert test GeoIP data + # 1.2.3.4 = 1*16777216 + 2*65536 + 3*256 + 4 = 16909060 + now = DateTime.utc_now(:second) + + Repo.insert!(%Location{ + geoname_id: 99_999, + country_code: "XX", + country_name: "Test Country", + city_name: "Test City", + subdivision_1_name: "Test State", + inserted_at: now, + updated_at: now + }) + + Repo.insert!(%Block{ + network: "1.2.3.0/24", + start_ip_int: 16_909_056, + end_ip_int: 16_909_311, + geoname_id: 99_999, + inserted_at: now, + updated_at: now + }) + + attrs = %{ + user_id: user.id, + email: user.email, + success: true, + method: "password", + ip_address: "1.2.3.4" + } + + assert {:ok, attempt} = Accounts.record_login_attempt(attrs) + assert attempt.country_code == "XX" + assert attempt.country_name == "Test Country" + assert attempt.city_name == "Test City" + assert attempt.subdivision_1_name == "Test State" + end + end + + describe "create_browser_session/1 with GeoIP data" do + test "enriches session with GeoIP location data" do + user = user_fixture() + {_token, user_token} = Accounts.generate_user_session_token_with_record(user) + now = DateTime.utc_now(:second) + + # Insert test GeoIP data for 2.3.4.5 + # 2.3.4.0 = 2*16777216 + 3*65536 + 4*256 = 33752064 + # 2.3.4.255 = 33752064 + 255 = 33752319 + Repo.insert!(%Location{ + geoname_id: 88_888, + country_code: "YY", + country_name: "Another Country", + city_name: "Another City", + subdivision_1_name: "Another State", + inserted_at: now, + updated_at: now + }) + + Repo.insert!(%Block{ + network: "2.3.4.0/24", + start_ip_int: 33_752_064, + end_ip_int: 33_752_319, + geoname_id: 88_888, + inserted_at: now, + updated_at: now + }) + + attrs = %{ + user_id: user.id, + user_token_id: user_token.id, + ip_address: "2.3.4.5", + user_agent: "TestBrowser/1.0", + last_activity_at: now, + expires_at: DateTime.add(now, 14, :day) + } + + assert {:ok, session} = Accounts.create_browser_session(attrs) + assert session.country_code == "YY" + assert session.country_name == "Another Country" + assert session.city_name == "Another City" + assert session.subdivision_1_name == "Another State" + end + end + + describe "verify_totp_only/2 rejects numeric recovery code match" do + test "returns recovery_code_not_allowed when numeric code matches a recovery code" do + # Create user with TOTP enabled + user = user_fixture(enable_totp: true) + assert Accounts.totp_enabled?(user) + + # Manually insert a recovery code whose hash matches the numeric string "123456" + # This simulates an unlikely scenario where a recovery code hash collides with a numeric input + numeric_code = "123456" + code_hash = UserRecoveryCode.hash_code(numeric_code) + now = DateTime.utc_now(:second) + + Repo.insert!(%UserRecoveryCode{ + user_id: user.id, + code_hash: code_hash, + created_at: now + }) + + # verify_totp_only sees "123456" as all-numeric, ≤ 6 digits, so passes it to verify_user_mfa + # TOTP check fails (wrong code), recovery code check succeeds (hash matches) + # verify_user_mfa returns {:ok, user, :recovery_code} + # verify_totp_only maps that to {:error, :recovery_code_not_allowed} + assert {:error, :recovery_code_not_allowed} = Accounts.verify_totp_only(user, numeric_code) + end + end + + describe "revoke_browser_session/3 edge cases" do + test "returns error when user_token has already been deleted" do + user = user_fixture() + {_token1, user_token1} = Accounts.generate_user_session_token_with_record(user) + {_token2, user_token2} = Accounts.generate_user_session_token_with_record(user) + + {:ok, _session1} = + Accounts.create_browser_session(%{ + user_id: user.id, + user_token_id: user_token1.id, + ip_address: "10.0.0.1", + user_agent: "TestBrowser/1.0", + last_activity_at: DateTime.utc_now(:second), + expires_at: DateTime.add(DateTime.utc_now(:second), 14, :day) + }) + + {:ok, session2} = + Accounts.create_browser_session(%{ + user_id: user.id, + user_token_id: user_token2.id, + ip_address: "10.0.0.2", + user_agent: "TestBrowser/2.0", + last_activity_at: DateTime.utc_now(:second), + expires_at: DateTime.add(DateTime.utc_now(:second), 14, :day) + }) + + # Delete the user_token directly + Repo.delete!(user_token2) + + # Now trying to revoke should return not_found (token gone) + assert {:error, :not_found} = + Accounts.revoke_browser_session(session2.id, user.id, user_token1.id) + end end end