From 200ca0b5f70d1a6bac41dc0adbf788997eaa8cae Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Wed, 28 Feb 2024 17:58:19 -0500 Subject: [PATCH 01/25] recovery codes before 2fa setup --- .../accounts/user_settings_controller.ex | 15 ++++++++++- lib/recognizer_web/router.ex | 1 + .../user_settings/recovery_codes.html.eex | 27 +++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 6a31a5e8..0bc09bdf 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -89,7 +89,20 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do Accounts.generate_and_cache_new_two_factor_settings(user, preference) - redirect(conn, to: Routes.user_settings_path(conn, :two_factor)) + redirect(conn, to: Routes.user_settings_path(conn, :review)) + end + + def review(conn, _params) do + user = Authentication.fetch_current_user(conn) + {:ok, %{recovery_codes: recovery_codes}} = Accounts.get_new_two_factor_settings(user) + + recovery_block = + recovery_codes + |> Enum.map(& &1.code) + |> Enum.map(& &1 <> "\n") + + conn + |> render("recovery_codes.html", recovery_block: recovery_block) end defp assign_email_and_password_changesets(conn, _opts) do diff --git a/lib/recognizer_web/router.ex b/lib/recognizer_web/router.ex index 9067892e..67ed6fbc 100644 --- a/lib/recognizer_web/router.ex +++ b/lib/recognizer_web/router.ex @@ -127,5 +127,6 @@ defmodule RecognizerWeb.Router do put "/settings", UserSettingsController, :update get "/settings/two-factor", UserSettingsController, :two_factor post "/settings/two-factor", UserSettingsController, :two_factor_confirm + get "/settings/two-factor/review", UserSettingsController, :review end end diff --git a/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex b/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex new file mode 100644 index 00000000..58fc0e3c --- /dev/null +++ b/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex @@ -0,0 +1,27 @@ +
+

Copy Recovery Codes

+ +
+

+ Recovery codes are used to access your account if you have lost access to your device. +

+ +

+ + Download, print or copy your recovery codes before continuing + two-factor authentication setup. + +

+
+ +
+ 
<%= @recovery_block %>
+
+ +
+
+ <%= link "Continue", to: Routes.user_settings_path(@conn, :two_factor), class: "button is-secondary" %> +
+
+ +
\ No newline at end of file From c423e72af7b4823601d8b137095a1f42e894ba9c Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Mon, 4 Mar 2024 13:55:22 -0500 Subject: [PATCH 02/25] fallback redirect for unfound codes --- .../accounts/user_settings_controller.ex | 21 ++++++++++++------- .../user_settings_controller_test.exs | 14 +++++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 0bc09bdf..b71bf9b7 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -94,15 +94,22 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do def review(conn, _params) do user = Authentication.fetch_current_user(conn) - {:ok, %{recovery_codes: recovery_codes}} = Accounts.get_new_two_factor_settings(user) - recovery_block = - recovery_codes - |> Enum.map(& &1.code) - |> Enum.map(& &1 <> "\n") + case Accounts.get_new_two_factor_settings(user) do + {:ok, %{recovery_codes: recovery_codes}} -> + recovery_block = + recovery_codes + |> Enum.map(& &1.code) + |> Enum.map(&(&1 <> "\n")) - conn - |> render("recovery_codes.html", recovery_block: recovery_block) + conn + |> render("recovery_codes.html", recovery_block: recovery_block) + + _ -> + conn + |> put_flash(:error, "Two factor setup not yet initiated") + |> redirect(to: Routes.user_settings_path(conn, :edit)) + end end defp assign_email_and_password_changesets(conn, _opts) do diff --git a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs index bd784dc3..f9dc37ed 100644 --- a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs +++ b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs @@ -112,4 +112,18 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do assert response =~ "must not contain special characters" end end + + describe "GET /users/settings/two-factor/review (backup codes)" do + test "gets review page after 2fa setup", %{conn: conn, user: user} do + Recognizer.Accounts.generate_and_cache_new_two_factor_settings(user, "app") + conn = get(conn, Routes.user_settings_path(conn, :review)) + _response = html_response(conn, 200) + end + + test "review 2fa without setup is ?", %{conn: conn} do + conn = get(conn, Routes.user_settings_path(conn, :review)) + _response = html_response(conn, 302) + assert get_flash(conn, :error) == "Two factor setup not yet initiated" + end + end end From 5af99d2a52589768b9db8f5031aae386bd086503 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Wed, 6 Mar 2024 17:09:01 -0500 Subject: [PATCH 03/25] cache expiry for account settings --- config/config.exs | 3 +++ config/dev.exs | 3 +++ config/releases.exs | 2 ++ lib/recognizer/accounts.ex | 17 +++++++++++++++-- .../accounts/user_settings_controller.ex | 2 +- test/recognizer/accounts_test.exs | 13 +++++++++++++ .../accounts/user_settings_controller_test.exs | 2 +- 7 files changed, 38 insertions(+), 4 deletions(-) diff --git a/config/config.exs b/config/config.exs index 4d092b4e..536d9a85 100644 --- a/config/config.exs +++ b/config/config.exs @@ -95,4 +95,7 @@ config :spandex_ecto, SpandexEcto.EctoLogger, config :spandex_phoenix, tracer: Recognizer.Tracer config :spandex, :decorators, tracer: Recognizer.Tracer +config :recognizer, Recognizer.Accounts, + cache_expiry: 60 * 15 + import_config "#{Mix.env()}.exs" diff --git a/config/dev.exs b/config/dev.exs index c00e8e21..2bc95311 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -63,3 +63,6 @@ config :recognizer, Recognizer.BigCommerce, logout_uri: "http://localhost/logout", http_client: HTTPoison, enabled?: false + +config :recognizer, Recognizer.Accounts, + cache_expiry: 60 * 60 * 24 * 7 diff --git a/config/releases.exs b/config/releases.exs index e92c5a9a..33aaaef0 100644 --- a/config/releases.exs +++ b/config/releases.exs @@ -78,3 +78,5 @@ config :recognizer, Recognizer.BigCommerce, logout_uri: recognizer_config["BIGCOMMERCE_LOGOUT_URI"], http_client: HTTPoison, enabled?: false + +config :recognizer, Recognizer.Accounts, cache_expiry: recognizer_config["ACCOUNT_CACHE_EXPIRY_SECONDS"] diff --git a/lib/recognizer/accounts.ex b/lib/recognizer/accounts.ex index 3ebe425e..ac172b22 100644 --- a/lib/recognizer/accounts.ex +++ b/lib/recognizer/accounts.ex @@ -610,7 +610,13 @@ defmodule Recognizer.Accounts do two_factor_enabled: true } - Redix.noreply_command(:redix, ["SET", "two_factor_settings:#{user.id}", Jason.encode!(attrs)]) + :ok = Redix.noreply_command(:redix, [ + "SET", + "two_factor_settings:#{user.id}", + Jason.encode!(attrs), + "EX", + config(:cache_expiry) + ]) attrs end @@ -639,7 +645,7 @@ defmodule Recognizer.Accounts do end @doc """ - Retreives the new user's two factor settings from our cache. These settings + Retrieves the new user's two factor settings from our cache. These settings are not yet active, but are in the process of being verified. """ def get_new_two_factor_settings(user) do @@ -665,6 +671,9 @@ defmodule Recognizer.Accounts do end end + def clear_two_factor_settings(user), do: + {:ok, _} = Redix.command(:redix, ["DEL", "two_factor_settings:#{user.id}"]) + def load_notification_preferences(user) do Repo.preload(user, :notification_preference) end @@ -738,4 +747,8 @@ defmodule Recognizer.Accounts do Repo.delete_all(user_codes) end + + defp config(key), do: + Application.get_env(:recognizer, __MODULE__)[key] + end diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index b71bf9b7..9805e0af 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -107,7 +107,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do _ -> conn - |> put_flash(:error, "Two factor setup not yet initiated") + |> put_flash(:error, "Two factor setup expired or not yet initiated") |> redirect(to: Routes.user_settings_path(conn, :edit)) end end diff --git a/test/recognizer/accounts_test.exs b/test/recognizer/accounts_test.exs index 20ec00c1..f38142c1 100644 --- a/test/recognizer/accounts_test.exs +++ b/test/recognizer/accounts_test.exs @@ -373,6 +373,19 @@ defmodule Recognizer.AccountsTest do end end + describe "two-factor" do + setup do + %{user: insert(:user)} + end + + test "settings cache lifecycle", %{user: user} do + Accounts.generate_and_cache_new_two_factor_settings(user, "app") + assert {:ok, %{}} = Accounts.get_new_two_factor_settings(user) + Accounts.clear_two_factor_settings(user) + assert {:ok, nil} = Accounts.get_new_two_factor_settings(user) + end + end + describe "inspect/2" do test "does not include password" do refute inspect(%User{password: "123456"}) =~ "password: \"123456\"" diff --git a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs index f9dc37ed..22651339 100644 --- a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs +++ b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs @@ -120,7 +120,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do _response = html_response(conn, 200) end - test "review 2fa without setup is ?", %{conn: conn} do + test "review 2fa without cached codes is redirected with flash error", %{conn: conn} do conn = get(conn, Routes.user_settings_path(conn, :review)) _response = html_response(conn, 302) assert get_flash(conn, :error) == "Two factor setup not yet initiated" From c2623a1d1bd40199c2875c80b8f4dbb469709b17 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Fri, 8 Mar 2024 13:58:52 -0500 Subject: [PATCH 04/25] doc, format --- config/config.exs | 3 +-- config/dev.exs | 3 +-- lib/recognizer/accounts.ex | 25 +++++++++++++------------ 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/config/config.exs b/config/config.exs index 536d9a85..f14b764e 100644 --- a/config/config.exs +++ b/config/config.exs @@ -95,7 +95,6 @@ config :spandex_ecto, SpandexEcto.EctoLogger, config :spandex_phoenix, tracer: Recognizer.Tracer config :spandex, :decorators, tracer: Recognizer.Tracer -config :recognizer, Recognizer.Accounts, - cache_expiry: 60 * 15 +config :recognizer, Recognizer.Accounts, cache_expiry: 60 * 15 import_config "#{Mix.env()}.exs" diff --git a/config/dev.exs b/config/dev.exs index 2bc95311..0c0c33ce 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -64,5 +64,4 @@ config :recognizer, Recognizer.BigCommerce, http_client: HTTPoison, enabled?: false -config :recognizer, Recognizer.Accounts, - cache_expiry: 60 * 60 * 24 * 7 +config :recognizer, Recognizer.Accounts, cache_expiry: 60 * 60 * 24 * 7 diff --git a/lib/recognizer/accounts.ex b/lib/recognizer/accounts.ex index ac172b22..c39146c2 100644 --- a/lib/recognizer/accounts.ex +++ b/lib/recognizer/accounts.ex @@ -610,13 +610,14 @@ defmodule Recognizer.Accounts do two_factor_enabled: true } - :ok = Redix.noreply_command(:redix, [ - "SET", - "two_factor_settings:#{user.id}", - Jason.encode!(attrs), - "EX", - config(:cache_expiry) - ]) + :ok = + Redix.noreply_command(:redix, [ + "SET", + "two_factor_settings:#{user.id}", + Jason.encode!(attrs), + "EX", + config(:cache_expiry) + ]) attrs end @@ -671,8 +672,10 @@ defmodule Recognizer.Accounts do end end - def clear_two_factor_settings(user), do: - {:ok, _} = Redix.command(:redix, ["DEL", "two_factor_settings:#{user.id}"]) + @doc """ + Deletes cached settings. + """ + def clear_two_factor_settings(user), do: {:ok, _} = Redix.command(:redix, ["DEL", "two_factor_settings:#{user.id}"]) def load_notification_preferences(user) do Repo.preload(user, :notification_preference) @@ -748,7 +751,5 @@ defmodule Recognizer.Accounts do Repo.delete_all(user_codes) end - defp config(key), do: - Application.get_env(:recognizer, __MODULE__)[key] - + defp config(key), do: Application.get_env(:recognizer, __MODULE__)[key] end From 62808297e012389abfaa9906aa1d80cbf9b8a74d Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 12 Mar 2024 16:28:23 -0400 Subject: [PATCH 05/25] fixed: cache clear and retry redirect * settings cache cleared on 2fa confirm * redirect to retry after confirming with incorrect code --- .../accounts/user_settings_controller.ex | 8 +-- .../user_settings_controller_test.exs | 50 ++++++++++++++++++- 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 9805e0af..6c6b4328 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -30,14 +30,16 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do case Accounts.confirm_and_save_two_factor_settings(two_factor_code, user) do {:ok, _updated_user} -> + Accounts.clear_two_factor_settings(user) + conn - |> put_flash(:info, "Two factor code verified.") + |> put_flash(:info, "Two factor code verified") |> redirect(to: Routes.user_settings_path(conn, :edit)) _ -> conn - |> put_flash(:error, "Two factor code is invalid.") - |> redirect(to: Routes.user_settings_path(conn, :confirm_two_factor)) + |> put_flash(:error, "Two factor code is invalid") + |> redirect(to: Routes.user_settings_path(conn, :two_factor_confirm)) end end diff --git a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs index 22651339..c7031c55 100644 --- a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs +++ b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs @@ -6,6 +6,10 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do import Recognizer.BigCommerceTestHelpers alias Recognizer.Accounts + alias Recognizer.Accounts.User + alias Recognizer.Repo + alias RecognizerWeb.Authentication + setup :register_and_log_in_user @@ -113,9 +117,11 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do end end + + describe "GET /users/settings/two-factor/review (backup codes)" do test "gets review page after 2fa setup", %{conn: conn, user: user} do - Recognizer.Accounts.generate_and_cache_new_two_factor_settings(user, "app") + Accounts.generate_and_cache_new_two_factor_settings(user, "app") conn = get(conn, Routes.user_settings_path(conn, :review)) _response = html_response(conn, 200) end @@ -123,7 +129,47 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do test "review 2fa without cached codes is redirected with flash error", %{conn: conn} do conn = get(conn, Routes.user_settings_path(conn, :review)) _response = html_response(conn, 302) - assert get_flash(conn, :error) == "Two factor setup not yet initiated" + assert get_flash(conn, :error) == "Two factor setup expired or not yet initiated" end end + + describe "GET /users/settings/two-factor (confirmation)" do + test "/two-factor page is rendered with settings", %{conn: conn, user: user} do + Accounts.generate_and_cache_new_two_factor_settings(user, "app") + conn = get(conn, Routes.user_settings_path(conn, :two_factor)) + assert html_response(conn, 200) =~ "Configure App" + end + + test "/two-factor/confirm saves and clears", %{conn: conn, user: user} do + settings = Accounts.generate_and_cache_new_two_factor_settings(user, "app") + + token = Authentication.generate_token(settings) + params = %{"two_factor_code" => token} + + conn = post(conn, Routes.user_settings_path(conn, :two_factor_confirm), params) + + assert redirected_to(conn) =~ "/settings" + assert get_flash(conn, :info) =~ "Two factor code verified" + + %{recovery_codes: recovery_codes} = + User + |> Repo.get(user.id) + |> Repo.preload(:recovery_codes) + + refute Enum.empty?(recovery_codes) + + assert {:ok, nil} = Accounts.get_new_two_factor_settings(user) + end + + test "/two-factor/confirm redirects without cached settings", %{conn: conn, user: user} do + settings = Accounts.generate_and_cache_new_two_factor_settings(user, "app") + token = Authentication.generate_token(settings) + Accounts.clear_two_factor_settings(user) + params = %{"two_factor_code" => token} + conn = post(conn, Routes.user_settings_path(conn, :two_factor_confirm), params) + assert redirected_to(conn) =~ "/two-factor" + assert get_flash(conn, :error) =~ "Two factor code is invalid" + end + end + end From f832ee27ac7988bfd5aab181fd7cf61bde040dd0 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 19 Mar 2024 14:39:54 -0400 Subject: [PATCH 06/25] copy recovery codes as "Copy and Continue" single button --- assets/scripts/main.js | 10 ++++++++++ assets/scripts/util.js | 6 ++++++ .../accounts/user_settings/recovery_codes.html.eex | 2 +- .../accounts/user_settings_controller_test.exs | 4 ---- 4 files changed, 17 insertions(+), 5 deletions(-) create mode 100644 assets/scripts/util.js diff --git a/assets/scripts/main.js b/assets/scripts/main.js index f12dc1ad..583b4c53 100644 --- a/assets/scripts/main.js +++ b/assets/scripts/main.js @@ -1,4 +1,5 @@ import '../../deps/phoenix_html/priv/static/phoenix_html.js' +import './util.js' import '../styles/main.scss' @@ -23,4 +24,13 @@ documentReady(function () { toggleDisplay('div.company_name') }) }) + + document + .querySelectorAll('#copy-text') + .forEach((field) => { + field.addEventListener('click', (event) => { + const text = event.target.dataset.recoveryBlock + copyClipboard(text) + }) + }) }) diff --git a/assets/scripts/util.js b/assets/scripts/util.js new file mode 100644 index 00000000..034cf43d --- /dev/null +++ b/assets/scripts/util.js @@ -0,0 +1,6 @@ + +function copyClipboard(text) { + navigator.clipboard.writeText(text) +} + +window.copyClipboard = copyClipboard diff --git a/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex b/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex index 58fc0e3c..40c2707a 100644 --- a/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex +++ b/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex @@ -20,7 +20,7 @@
- <%= link "Continue", to: Routes.user_settings_path(@conn, :two_factor), class: "button is-secondary" %> + <%= link "Copy and Continue", to: Routes.user_settings_path(@conn, :two_factor), class: "button is-secondary", id: "copy-text", data: [recovery_block: @recovery_block] %>
diff --git a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs index c7031c55..426b995d 100644 --- a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs +++ b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs @@ -10,7 +10,6 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do alias Recognizer.Repo alias RecognizerWeb.Authentication - setup :register_and_log_in_user describe "GET /users/settings" do @@ -117,8 +116,6 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do end end - - describe "GET /users/settings/two-factor/review (backup codes)" do test "gets review page after 2fa setup", %{conn: conn, user: user} do Accounts.generate_and_cache_new_two_factor_settings(user, "app") @@ -171,5 +168,4 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do assert get_flash(conn, :error) =~ "Two factor code is invalid" end end - end From 614ead292ab0fa778622877762ae1294d3031933 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 19 Mar 2024 15:23:20 -0400 Subject: [PATCH 07/25] join --- .../controllers/accounts/user_settings_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 6c6b4328..e5a06989 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -102,7 +102,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do recovery_block = recovery_codes |> Enum.map(& &1.code) - |> Enum.map(&(&1 <> "\n")) + |> Enum.join("\n") conn |> render("recovery_codes.html", recovery_block: recovery_block) From f6622096cb0892f3ccc44de5b0cc3693fcdc9c2f Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 13 Feb 2024 15:57:40 -0500 Subject: [PATCH 08/25] don't redirect if bc session --- .../controllers/accounts/user_settings_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index e5a06989..6f946143 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -7,7 +7,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do plug :assign_email_and_password_changesets def edit(conn, _params) do - if Application.get_env(:recognizer, :redirect_url) do + if Application.get_env(:recognizer, :redirect_url) && !get_session(conn, :bc) do redirect(conn, external: Application.get_env(:recognizer, :redirect_url)) else render(conn, "edit.html") From 0fe1afe3e1686c988dc429e75ad1877597a1c95f Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 13 Feb 2024 18:09:26 -0500 Subject: [PATCH 09/25] csp for frame ancestors --- .../controllers/accounts/user_settings_controller.ex | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 6f946143..77277a81 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -10,7 +10,12 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do if Application.get_env(:recognizer, :redirect_url) && !get_session(conn, :bc) do redirect(conn, external: Application.get_env(:recognizer, :redirect_url)) else - render(conn, "edit.html") + conn + |> put_resp_header( + "Content-Security-Policy", + "default-src 'self'; frame-ancestors 'self' https://bigcommerce.com;" + ) + |> render("edit.html") end end From 35340e5ec36867b12ffc33b2b98ded4931955fd8 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 13 Feb 2024 18:18:47 -0500 Subject: [PATCH 10/25] delete x-frame-options header --- .../controllers/accounts/user_settings_controller.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 77277a81..c8f98764 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -11,6 +11,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do redirect(conn, external: Application.get_env(:recognizer, :redirect_url)) else conn + |> delete_resp_header("x-frame-options") |> put_resp_header( "Content-Security-Policy", "default-src 'self'; frame-ancestors 'self' https://bigcommerce.com;" From 942ee4ca7177bfe568c5784a6efaa4bb0a8fe233 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Wed, 14 Feb 2024 11:55:39 -0500 Subject: [PATCH 11/25] in :browser pipeline instead --- .../controllers/accounts/user_settings_controller.ex | 8 +------- lib/recognizer_web/router.ex | 10 ++++++++++ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index c8f98764..6f946143 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -10,13 +10,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do if Application.get_env(:recognizer, :redirect_url) && !get_session(conn, :bc) do redirect(conn, external: Application.get_env(:recognizer, :redirect_url)) else - conn - |> delete_resp_header("x-frame-options") - |> put_resp_header( - "Content-Security-Policy", - "default-src 'self'; frame-ancestors 'self' https://bigcommerce.com;" - ) - |> render("edit.html") + render(conn, "edit.html") end end diff --git a/lib/recognizer_web/router.ex b/lib/recognizer_web/router.ex index 67ed6fbc..293913d7 100644 --- a/lib/recognizer_web/router.ex +++ b/lib/recognizer_web/router.ex @@ -11,6 +11,7 @@ defmodule RecognizerWeb.Router do plug :fetch_flash plug :protect_from_forgery plug :put_secure_browser_headers, @hsts_header + plug :allow_bc_frame end pipeline :api do @@ -45,6 +46,15 @@ defmodule RecognizerWeb.Router do conn end + defp allow_bc_frame(conn, _opts), + do: + conn + |> delete_resp_header("x-frame-options") + |> put_resp_header( + "Content-Security-Policy", + "default-src 'self'; frame-ancestors 'self' https://bigcommerce.com;" + ) + scope "/", RecognizerWeb do pipe_through [:browser, :bc] From e9c4a6025fc0eac003331489b91e0d5df310fb83 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Wed, 14 Feb 2024 12:14:23 -0500 Subject: [PATCH 12/25] allowed url --- lib/recognizer_web/router.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/recognizer_web/router.ex b/lib/recognizer_web/router.ex index 293913d7..5d4970af 100644 --- a/lib/recognizer_web/router.ex +++ b/lib/recognizer_web/router.ex @@ -52,7 +52,7 @@ defmodule RecognizerWeb.Router do |> delete_resp_header("x-frame-options") |> put_resp_header( "Content-Security-Policy", - "default-src 'self'; frame-ancestors 'self' https://bigcommerce.com;" + "default-src 'self'; frame-ancestors 'self' https://system76.mybigcommerce.com;" ) scope "/", RecognizerWeb do From a3c39daa910a7429f1740a7919c07d33f1339088 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Wed, 21 Feb 2024 12:12:36 -0500 Subject: [PATCH 13/25] undo window changes --- lib/recognizer_web/router.ex | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/lib/recognizer_web/router.ex b/lib/recognizer_web/router.ex index 5d4970af..67ed6fbc 100644 --- a/lib/recognizer_web/router.ex +++ b/lib/recognizer_web/router.ex @@ -11,7 +11,6 @@ defmodule RecognizerWeb.Router do plug :fetch_flash plug :protect_from_forgery plug :put_secure_browser_headers, @hsts_header - plug :allow_bc_frame end pipeline :api do @@ -46,15 +45,6 @@ defmodule RecognizerWeb.Router do conn end - defp allow_bc_frame(conn, _opts), - do: - conn - |> delete_resp_header("x-frame-options") - |> put_resp_header( - "Content-Security-Policy", - "default-src 'self'; frame-ancestors 'self' https://system76.mybigcommerce.com;" - ) - scope "/", RecognizerWeb do pipe_through [:browser, :bc] From f01a1a6b0873da4a17207fd71d28c04dce3e623a Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Thu, 22 Feb 2024 17:42:41 -0500 Subject: [PATCH 14/25] optionally redirect other 2fa methods --- .../accounts/user_settings_controller.ex | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 6f946143..6c2bc3a5 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -16,12 +16,22 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do def two_factor(conn, _params) do user = Authentication.fetch_current_user(conn) - {:ok, %{two_factor_seed: seed}} = Accounts.get_new_two_factor_settings(user) - render(conn, "confirm_two_factor.html", - barcode: Authentication.generate_totp_barcode(user, seed), - totp_app_url: Authentication.get_totp_app_url(user, seed) - ) + case Accounts.get_new_two_factor_settings(user) do + {:ok, %{two_factor_seed: seed, notification_preference: %{two_factor: "app"}}} -> + render(conn, "confirm_two_factor.html", + barcode: Authentication.generate_totp_barcode(user, seed), + totp_app_url: Authentication.get_totp_app_url(user, seed) + ) + + {:ok, settings} -> + :ok = Accounts.send_new_two_factor_notification(user, settings) + + conn + |> put_session(:two_factor_user_id, user.id) + |> put_session(:two_factor_sent, false) + |> redirect(to: Routes.user_two_factor_path(conn, :new)) + end end def two_factor_confirm(conn, params) do From 005ed7b890921eeecc557e0b897d118af09ca191 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Fri, 23 Feb 2024 15:56:49 -0500 Subject: [PATCH 15/25] (wip) stub for later --- .../accounts/user_settings_controller.ex | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 6c2bc3a5..e01bad67 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -17,20 +17,21 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do def two_factor(conn, _params) do user = Authentication.fetch_current_user(conn) + # TODO params instead of cache case Accounts.get_new_two_factor_settings(user) do {:ok, %{two_factor_seed: seed, notification_preference: %{two_factor: "app"}}} -> + IO.puts("rendering app 2fa confirmation...") + render(conn, "confirm_two_factor.html", barcode: Authentication.generate_totp_barcode(user, seed), totp_app_url: Authentication.get_totp_app_url(user, seed) ) - {:ok, settings} -> - :ok = Accounts.send_new_two_factor_notification(user, settings) + {:ok, _} -> + IO.puts("sending external 2fa notification...") - conn - |> put_session(:two_factor_user_id, user.id) - |> put_session(:two_factor_sent, false) - |> redirect(to: Routes.user_two_factor_path(conn, :new)) + # TODO not this path, a new page.. + redirect(conn, to: Routes.user_two_factor_path(conn, :new)) end end From 7f49d43c970ea6cd61fd76623e0a86dd987bc5f3 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Fri, 22 Mar 2024 15:10:15 -0400 Subject: [PATCH 16/25] wip voice/text 2fa setup --- .../accounts/user_settings_controller.ex | 39 +++++++++++-------- .../accounts/user_two_factor_controller.ex | 5 ++- lib/recognizer_web/router.ex | 2 +- .../confirm_two_factor_external.html.eex | 26 +++++++++++++ 4 files changed, 53 insertions(+), 19 deletions(-) create mode 100644 lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index e01bad67..7f1bcfda 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -13,28 +13,30 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do render(conn, "edit.html") end end - - def two_factor(conn, _params) do + @doc """ + Generate codes for a new two factor setup + """ + def two_factor_init(conn, _params) do user = Authentication.fetch_current_user(conn) + {:ok, %{two_factor_seed: seed, notification_preference: %{two_factor: method}} = settings} = Accounts.get_new_two_factor_settings(user) - # TODO params instead of cache - case Accounts.get_new_two_factor_settings(user) do - {:ok, %{two_factor_seed: seed, notification_preference: %{two_factor: "app"}}} -> - IO.puts("rendering app 2fa confirmation...") - - render(conn, "confirm_two_factor.html", - barcode: Authentication.generate_totp_barcode(user, seed), - totp_app_url: Authentication.get_totp_app_url(user, seed) - ) - - {:ok, _} -> - IO.puts("sending external 2fa notification...") - - # TODO not this path, a new page.. - redirect(conn, to: Routes.user_two_factor_path(conn, :new)) + if method == "text" || method == "voice" do + # TODO error without user phone + # TODO rate limit, retry button + :ok = Accounts.send_new_two_factor_notification(user, settings) + render(conn, "confirm_two_factor_external.html") + else + render(conn, "confirm_two_factor.html", + barcode: Authentication.generate_totp_barcode(user, seed), + totp_app_url: Authentication.get_totp_app_url(user, seed) + ) end + end + @doc """ + Confirming and saving a new two factor setup with user-provided code + """ def two_factor_confirm(conn, params) do two_factor_code = Map.get(params, "two_factor_code", "") user = Authentication.fetch_current_user(conn) @@ -96,6 +98,9 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do end end + @doc """ + Form submission to begin two factor setup + """ def update(conn, %{"action" => "update_two_factor", "user" => user_params}) do user = Authentication.fetch_current_user(conn) preference = get_in(user_params, ["notification_preference", "two_factor"]) diff --git a/lib/recognizer_web/controllers/accounts/user_two_factor_controller.ex b/lib/recognizer_web/controllers/accounts/user_two_factor_controller.ex index 349dfae8..4806d1f9 100644 --- a/lib/recognizer_web/controllers/accounts/user_two_factor_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_two_factor_controller.ex @@ -25,6 +25,9 @@ defmodule RecognizerWeb.Accounts.UserTwoFactorController do ] when action in [:resend] + @doc """ + Prompt the user for a two factor code on login + """ def new(conn, _params) do current_user_id = get_session(conn, :two_factor_user_id) current_user = Accounts.get_user!(current_user_id) @@ -37,7 +40,7 @@ defmodule RecognizerWeb.Accounts.UserTwoFactorController do end @doc """ - Handle a user creating a session with a two factor code + Verify a user creating a session with a two factor code """ def create(conn, %{"user" => %{"two_factor_code" => two_factor_code}}) do current_user_id = get_session(conn, :two_factor_user_id) diff --git a/lib/recognizer_web/router.ex b/lib/recognizer_web/router.ex index 67ed6fbc..e7f74fd6 100644 --- a/lib/recognizer_web/router.ex +++ b/lib/recognizer_web/router.ex @@ -125,7 +125,7 @@ defmodule RecognizerWeb.Router do get "/settings", UserSettingsController, :edit put "/settings", UserSettingsController, :update - get "/settings/two-factor", UserSettingsController, :two_factor + get "/settings/two-factor", UserSettingsController, :two_factor_init post "/settings/two-factor", UserSettingsController, :two_factor_confirm get "/settings/two-factor/review", UserSettingsController, :review end diff --git a/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex b/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex new file mode 100644 index 00000000..9896378e --- /dev/null +++ b/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex @@ -0,0 +1,26 @@ +
+

Confirm Two Factor Authentication

+ +
+

If you did not receive a verification code, or it is expired, you can retry...(init)

+
+ +
+

Enter the provided 6-digit code:

+
+ + <%= form_for @conn, Routes.user_settings_path(@conn, :two_factor_confirm), fn f -> %> +
+
+ <%= text_input f, :two_factor_code, inputmode: "numeric", pattern: "[0-9]*", autocomplete: "one-time-code", required: true, class: "is-medium #{input_classes(f, :two_factor_code)}" %> +
+ + <%= error_tag f, :two_factor_code %> +
+ +
+ <%= submit "Verify Code", class: "button is-secondary" %> + Cancel +
+ <% end %> +
From 56a7430667274c4706658a45ffacbd22b19685b4 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Fri, 22 Mar 2024 16:19:28 -0400 Subject: [PATCH 17/25] retry, rate limit --- .../controllers/accounts/user_settings_controller.ex | 12 +++++++++++- .../confirm_two_factor_external.html.eex | 3 ++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 7f1bcfda..62dbea88 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -4,8 +4,17 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do alias Recognizer.Accounts alias RecognizerWeb.Authentication + @one_minute 60_000 + plug :assign_email_and_password_changesets + plug Hammer.Plug, + [ + rate_limit: {"user_settings:two_factor", @one_minute, 2}, + by: {:conn, &get_user_id_from_request/1} + ] + when action in [:two_factor_init] + def edit(conn, _params) do if Application.get_env(:recognizer, :redirect_url) && !get_session(conn, :bc) do redirect(conn, external: Application.get_env(:recognizer, :redirect_url)) @@ -13,6 +22,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do render(conn, "edit.html") end end + @doc """ Generate codes for a new two factor setup """ @@ -22,7 +32,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do if method == "text" || method == "voice" do # TODO error without user phone - # TODO rate limit, retry button + # TODO better rate limit error :ok = Accounts.send_new_two_factor_notification(user, settings) render(conn, "confirm_two_factor_external.html") else diff --git a/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex b/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex index 9896378e..f884ef47 100644 --- a/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex +++ b/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex @@ -2,7 +2,8 @@

Confirm Two Factor Authentication

-

If you did not receive a verification code, or it is expired, you can retry...(init)

+

If you did not receive a verification code, or it has expired, you can retry:

+ <%= link "Send again", to: Routes.user_settings_path(@conn, :two_factor_init), class: "button is-secondary"%>
From 8f292111fb4d0ac970aa460075d64701403ee669 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Fri, 22 Mar 2024 17:11:39 -0400 Subject: [PATCH 18/25] flash error for rate limited 2fa retries --- .../accounts/user_settings_controller.ex | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 62dbea88..5cf68048 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -11,7 +11,8 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do plug Hammer.Plug, [ rate_limit: {"user_settings:two_factor", @one_minute, 2}, - by: {:conn, &get_user_id_from_request/1} + by: {:conn, &get_user_id_from_request/1}, + on_deny: &__MODULE__.two_factor_rate_limited/2 ] when action in [:two_factor_init] @@ -28,11 +29,12 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do """ def two_factor_init(conn, _params) do user = Authentication.fetch_current_user(conn) - {:ok, %{two_factor_seed: seed, notification_preference: %{two_factor: method}} = settings} = Accounts.get_new_two_factor_settings(user) + + {:ok, %{two_factor_seed: seed, notification_preference: %{two_factor: method}} = settings} = + Accounts.get_new_two_factor_settings(user) if method == "text" || method == "voice" do # TODO error without user phone - # TODO better rate limit error :ok = Accounts.send_new_two_factor_notification(user, settings) render(conn, "confirm_two_factor_external.html") else @@ -41,7 +43,16 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do totp_app_url: Authentication.get_totp_app_url(user, seed) ) end + end + @doc """ + Graceful error for 2fa retry rate limits + """ + def two_factor_rate_limited(conn, _params) do + conn + |> put_flash(:error, "Too many requests, please wait and try again") + |> render( "confirm_two_factor_external.html") + |> halt() end @doc """ From 6b6f5031506252a8effc28de2e8eb214fc3ee67a Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Mon, 25 Mar 2024 09:02:17 -0400 Subject: [PATCH 19/25] clarity & color --- .../confirm_two_factor_external.html.eex | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex b/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex index f884ef47..4053e4e3 100644 --- a/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex +++ b/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex @@ -1,11 +1,6 @@

Confirm Two Factor Authentication

-
-

If you did not receive a verification code, or it has expired, you can retry:

- <%= link "Send again", to: Routes.user_settings_path(@conn, :two_factor_init), class: "button is-secondary"%> -
-

Enter the provided 6-digit code:

@@ -23,5 +18,13 @@ <%= submit "Verify Code", class: "button is-secondary" %> Cancel
+ +
+

If you did not receive a verification code, or it has expired:

+
+ <%= link "Send another", to: Routes.user_settings_path(@conn, :two_factor_init), class: "button is-warning is-right"%> +
+
+ <% end %>
From d4b13b2fa0e7f31a474e74c9e340564bdaded6eb Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Mon, 25 Mar 2024 11:52:47 -0400 Subject: [PATCH 20/25] rebased, fix function name --- .../templates/accounts/user_settings/recovery_codes.html.eex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex b/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex index 40c2707a..7440906c 100644 --- a/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex +++ b/lib/recognizer_web/templates/accounts/user_settings/recovery_codes.html.eex @@ -20,7 +20,7 @@
- <%= link "Copy and Continue", to: Routes.user_settings_path(@conn, :two_factor), class: "button is-secondary", id: "copy-text", data: [recovery_block: @recovery_block] %> + <%= link "Copy and Continue", to: Routes.user_settings_path(@conn, :two_factor_init), class: "button is-secondary", id: "copy-text", data: [recovery_block: @recovery_block] %>
From b73456e63a5d52d80d6ed1dd97915f8b1477ee0b Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Mon, 25 Mar 2024 17:36:31 -0400 Subject: [PATCH 21/25] rate limiting only for text/voice test arrangement, clarity --- .../accounts/user_settings_controller.ex | 22 +++++++++++-- lib/recognizer_web/router.ex | 2 +- .../user_settings_controller_test.exs | 33 +++++++++++++++---- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 5cf68048..b444a0b1 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -11,7 +11,8 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do plug Hammer.Plug, [ rate_limit: {"user_settings:two_factor", @one_minute, 2}, - by: {:conn, &get_user_id_from_request/1}, + by: {:conn, &__MODULE__.two_factor_rate_key/1}, + when_nil: :pass, on_deny: &__MODULE__.two_factor_rate_limited/2 ] when action in [:two_factor_init] @@ -34,7 +35,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do Accounts.get_new_two_factor_settings(user) if method == "text" || method == "voice" do - # TODO error without user phone + # TODO error without user phone (redirect here with flash error, or stop at settings update) :ok = Accounts.send_new_two_factor_notification(user, settings) render(conn, "confirm_two_factor_external.html") else @@ -45,13 +46,28 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do end end + @doc """ + Rate limit 2fa setup only for text & voice, bypass for app. + """ + def two_factor_rate_key(conn) do + user = Authentication.fetch_current_user(conn) + + case Accounts.get_new_two_factor_settings(user) do + {:ok, %{notification_preference: %{two_factor: "app"}}} -> + nil + + _ -> + get_user_id_from_request(conn) + end + end + @doc """ Graceful error for 2fa retry rate limits """ def two_factor_rate_limited(conn, _params) do conn |> put_flash(:error, "Too many requests, please wait and try again") - |> render( "confirm_two_factor_external.html") + |> render("confirm_two_factor_external.html") |> halt() end diff --git a/lib/recognizer_web/router.ex b/lib/recognizer_web/router.ex index e7f74fd6..8e9d0071 100644 --- a/lib/recognizer_web/router.ex +++ b/lib/recognizer_web/router.ex @@ -125,8 +125,8 @@ defmodule RecognizerWeb.Router do get "/settings", UserSettingsController, :edit put "/settings", UserSettingsController, :update + get "/settings/two-factor/review", UserSettingsController, :review get "/settings/two-factor", UserSettingsController, :two_factor_init post "/settings/two-factor", UserSettingsController, :two_factor_confirm - get "/settings/two-factor/review", UserSettingsController, :review end end diff --git a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs index 426b995d..438db301 100644 --- a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs +++ b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs @@ -116,11 +116,14 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do end end + # todo test "requires account phone number for text" + # TODO another for app bypass + describe "GET /users/settings/two-factor/review (backup codes)" do test "gets review page after 2fa setup", %{conn: conn, user: user} do Accounts.generate_and_cache_new_two_factor_settings(user, "app") conn = get(conn, Routes.user_settings_path(conn, :review)) - _response = html_response(conn, 200) + assert html_response(conn, 200) =~ "copy your recovery codes" end test "review 2fa without cached codes is redirected with flash error", %{conn: conn} do @@ -130,14 +133,32 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do end end - describe "GET /users/settings/two-factor (confirmation)" do - test "/two-factor page is rendered with settings", %{conn: conn, user: user} do + describe "GET /users/settings/two-factor (init)" do + test "/two-factor page is rendered for with settings for app, doesn't rate limit", %{conn: conn, user: user} do Accounts.generate_and_cache_new_two_factor_settings(user, "app") - conn = get(conn, Routes.user_settings_path(conn, :two_factor)) + conn = get(conn, Routes.user_settings_path(conn, :two_factor_init)) assert html_response(conn, 200) =~ "Configure App" + result2 = get(conn, Routes.user_settings_path(conn, :two_factor_init)) + assert html_response(result2, 200) =~ "Configure App" + result3 = get(conn, Routes.user_settings_path(conn, :two_factor_init)) + assert html_response(result3, 200) =~ "Configure App" + refute get_flash(result3, :error) + end + + test "/two-factor loads for text, limits retries", %{conn: conn, user: user} do + Accounts.generate_and_cache_new_two_factor_settings(user, "text") + result1 = get(conn, Routes.user_settings_path(conn, :two_factor_init)) + assert html_response(result1, 200) =~ "Enter the provided 6-digit code" + result2 = get(conn, Routes.user_settings_path(conn, :two_factor_init)) + assert html_response(result2, 200) =~ "Enter the provided 6-digit code" + result3 = get(conn, Routes.user_settings_path(conn, :two_factor_init)) + assert html_response(result3, 200) =~ "Enter the provided 6-digit code" + assert get_flash(result3, :error) =~ "Too many requests" end + end - test "/two-factor/confirm saves and clears", %{conn: conn, user: user} do + describe "POST /users/settings/two-factor (confirm)" do + test "confirm saves and clears cache", %{conn: conn, user: user} do settings = Accounts.generate_and_cache_new_two_factor_settings(user, "app") token = Authentication.generate_token(settings) @@ -158,7 +179,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do assert {:ok, nil} = Accounts.get_new_two_factor_settings(user) end - test "/two-factor/confirm redirects without cached settings", %{conn: conn, user: user} do + test "confirm redirects without cached settings", %{conn: conn, user: user} do settings = Accounts.generate_and_cache_new_two_factor_settings(user, "app") token = Authentication.generate_token(settings) Accounts.clear_two_factor_settings(user) From 800805859cca04f9dc01e29d59327cf92e65be01 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 26 Mar 2024 13:36:53 -0400 Subject: [PATCH 22/25] phone number required for text/voice 2fa with flash error redirect --- .../accounts/notification_preference.ex | 2 +- .../accounts/user_settings_controller.ex | 32 +++++++++++++------ .../user_settings_controller_test.exs | 31 ++++++++++++++++-- 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/lib/recognizer/accounts/notification_preference.ex b/lib/recognizer/accounts/notification_preference.ex index 76306af4..23acf154 100644 --- a/lib/recognizer/accounts/notification_preference.ex +++ b/lib/recognizer/accounts/notification_preference.ex @@ -9,7 +9,7 @@ defmodule Recognizer.Accounts.NotificationPreference do alias __MODULE__, as: NotificationPreference schema "notification_preferences" do - field :two_factor, Recognizer.TwoFactorPreference, default: :text + field :two_factor, Recognizer.TwoFactorPreference, default: :app belongs_to :user, User diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index b444a0b1..4ab222e9 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -35,7 +35,6 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do Accounts.get_new_two_factor_settings(user) if method == "text" || method == "voice" do - # TODO error without user phone (redirect here with flash error, or stop at settings update) :ok = Accounts.send_new_two_factor_notification(user, settings) render(conn, "confirm_two_factor_external.html") else @@ -93,6 +92,9 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do end end + @doc """ + Form submission for settings applied + """ def update(conn, %{"action" => "update", "user" => user_params}) do user = Authentication.fetch_current_user(conn) @@ -125,6 +127,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do end end + # disable 2fa def update(conn, %{"action" => "update_two_factor", "user" => %{"two_factor_enabled" => "0"}}) do user = Authentication.fetch_current_user(conn) @@ -135,18 +138,27 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do end end - @doc """ - Form submission to begin two factor setup - """ - def update(conn, %{"action" => "update_two_factor", "user" => user_params}) do - user = Authentication.fetch_current_user(conn) - preference = get_in(user_params, ["notification_preference", "two_factor"]) - - Accounts.generate_and_cache_new_two_factor_settings(user, preference) + # enable 2fa + def update(conn, %{ + "action" => "update_two_factor", + "user" => %{"notification_preference" => %{"two_factor" => preference}} + }) do + %{phone_number: phone_number} = user = Authentication.fetch_current_user(conn) - redirect(conn, to: Routes.user_settings_path(conn, :review)) + # phone number required for text/voice + if (preference == "text" || preference == "voice") && phone_number == nil do + conn + |> put_flash(:error, "Phone number required for text and voice two-factor methods") + |> redirect(to: Routes.user_settings_path(conn, :edit)) + else + Accounts.generate_and_cache_new_two_factor_settings(user, preference) + redirect(conn, to: Routes.user_settings_path(conn, :review)) + end end + @doc """ + Review recovery codes for copying. + """ def review(conn, _params) do user = Authentication.fetch_current_user(conn) diff --git a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs index 438db301..f00b4911 100644 --- a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs +++ b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs @@ -114,10 +114,35 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do assert response =~ "Change Profile" assert response =~ "must not contain special characters" end - end - # todo test "requires account phone number for text" - # TODO another for app bypass + test "update two-factor redirects for text method without phone number", %{conn: conn, user: user} do + stub(HTTPoisonMock, :put, fn _, _, _ -> ok_bigcommerce_response() end) + Accounts.update_user(user, %{phone_number: nil}) + + conn = + put(conn, Routes.user_settings_path(conn, :update), %{ + "action" => "update_two_factor", + "user" => %{"notification_preference" => %{"two_factor" => "text"}} + }) + + assert redirected_to(conn) =~ "/settings" + assert get_flash(conn, :error) =~ "Phone number required" + end + + test "update two-factor allows app setup without a phone number", %{conn: conn, user: user} do + stub(HTTPoisonMock, :put, fn _, _, _ -> ok_bigcommerce_response() end) + Accounts.update_user(user, %{phone_number: nil}) + + conn = + put(conn, Routes.user_settings_path(conn, :edit), %{ + "action" => "update_two_factor", + "user" => %{"notification_preference" => %{"two_factor" => "app"}} + }) + + assert redirected_to(conn) =~ "/settings/two-factor/review" + refute get_flash(conn, :error) + end + end describe "GET /users/settings/two-factor/review (backup codes)" do test "gets review page after 2fa setup", %{conn: conn, user: user} do From 05b7c2935827e8c5ab6a943929d0e2f9dcca1b7d Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 26 Mar 2024 14:08:50 -0400 Subject: [PATCH 23/25] map_join for credo --- .../controllers/accounts/user_settings_controller.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex index 4ab222e9..1a5801ea 100644 --- a/lib/recognizer_web/controllers/accounts/user_settings_controller.ex +++ b/lib/recognizer_web/controllers/accounts/user_settings_controller.ex @@ -166,8 +166,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do {:ok, %{recovery_codes: recovery_codes}} -> recovery_block = recovery_codes - |> Enum.map(& &1.code) - |> Enum.join("\n") + |> Enum.map_join("\n", & &1.code) conn |> render("recovery_codes.html", recovery_block: recovery_block) From 875287cb6571e2a995d60db56ad5e8a5a2035330 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 26 Mar 2024 14:33:48 -0400 Subject: [PATCH 24/25] wording, placement --- .../accounts/user_settings/edit.html.eex | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/recognizer_web/templates/accounts/user_settings/edit.html.eex b/lib/recognizer_web/templates/accounts/user_settings/edit.html.eex index ccd2f395..9ef5b549 100644 --- a/lib/recognizer_web/templates/accounts/user_settings/edit.html.eex +++ b/lib/recognizer_web/templates/accounts/user_settings/edit.html.eex @@ -14,7 +14,7 @@
-

Change Profile

+

Update Profile

<%= form_for @changeset, Routes.user_settings_path(@conn, :update), fn f -> %> <%= hidden_input f, :action, name: "action", value: "update" %> @@ -128,51 +128,51 @@
- <%= submit "Change Profile", class: "button is-secondary" %> + <%= submit "Update Profile", class: "button is-secondary" %>
<% end %>
-

Change Password

+

Update Password

<%= form_for @password_changeset, Routes.user_settings_path(@conn, :update), fn f -> %> <%= hidden_input f, :action, name: "action", value: "update_password" %>
- <%= label f, :password, "New Password", class: "label" %> + <%= label f, :current_password, class: "label" %>
- <%= password_input f, :password, class: input_classes(f, :password), required: true %> + <%= password_input f, :current_password, class: input_classes(f, :password), required: true, name: "current_password" %>
- <%= error_tag f, :password %> + <%= error_tag f, :current_password %>
- <%= label f, :password_confirmation, "Confirm new password", class: "label" %> + <%= label f, :password, "New Password", class: "label" %>
- <%= password_input f, :password_confirmation, class: input_classes(f, :password_confirmation), required: true %> + <%= password_input f, :password, class: input_classes(f, :password), required: true %>
- <%= error_tag f, :password_confirmation %> + <%= error_tag f, :password %>
- <%= label f, :current_password, class: "label" %> + <%= label f, :password_confirmation, "Confirm new password", class: "label" %>
- <%= password_input f, :current_password, class: input_classes(f, :password), required: true, name: "current_password" %> + <%= password_input f, :password_confirmation, class: input_classes(f, :password_confirmation), required: true %>
- <%= error_tag f, :current_password %> + <%= error_tag f, :password_confirmation %>
- <%= submit "Change Password", class: "button is-secondary" %> + <%= submit "Update Password", class: "button is-secondary" %>
<% end %> From 5c2703e06d13fd6e24fa1471e4abc4f9b1b3f590 Mon Sep 17 00:00:00 2001 From: Andrew Hebert Date: Tue, 26 Mar 2024 14:44:25 -0400 Subject: [PATCH 25/25] tests for wording --- .../accounts/user_settings_controller_test.exs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs index f00b4911..48d277ea 100644 --- a/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs +++ b/test/recognizer_web/controllers/accounts/user_settings_controller_test.exs @@ -55,7 +55,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do }) response = html_response(old_password_conn, 200) - assert response =~ "Change Password" + assert response =~ "Update Password" assert response =~ "must contain a number" assert response =~ "does not match password" assert response =~ "is not valid" @@ -87,7 +87,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do }) response = html_response(conn, 200) - assert response =~ "Change Profile" + assert response =~ "Update Profile" assert response =~ "must have the @ sign, no spaces and a top level domain" end @@ -99,7 +99,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do }) response = html_response(conn, 200) - assert response =~ "Change Profile" + assert response =~ "Update Profile" assert response =~ "must not contain special characters" end @@ -111,7 +111,7 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do }) response = html_response(conn, 200) - assert response =~ "Change Profile" + assert response =~ "Update Profile" assert response =~ "must not contain special characters" end