From f6622096cb0892f3ccc44de5b0cc3693fcdc9c2f Mon Sep 17 00:00:00 2001
From: Andrew Hebert <andrew@system76.com>
Date: Tue, 13 Feb 2024 15:57:40 -0500
Subject: [PATCH 01/15] 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 e5a0698..6f94614 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 <andrew@system76.com>
Date: Tue, 13 Feb 2024 18:09:26 -0500
Subject: [PATCH 02/15] 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 6f94614..77277a8 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 <andrew@system76.com>
Date: Tue, 13 Feb 2024 18:18:47 -0500
Subject: [PATCH 03/15] 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 77277a8..c8f9876 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 <andrew@system76.com>
Date: Wed, 14 Feb 2024 11:55:39 -0500
Subject: [PATCH 04/15] 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 c8f9876..6f94614 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 67ed6fb..293913d 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 <andrew@system76.com>
Date: Wed, 14 Feb 2024 12:14:23 -0500
Subject: [PATCH 05/15] 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 293913d..5d4970a 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 <andrew@system76.com>
Date: Wed, 21 Feb 2024 12:12:36 -0500
Subject: [PATCH 06/15] 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 5d4970a..67ed6fb 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 <andrew@system76.com>
Date: Thu, 22 Feb 2024 17:42:41 -0500
Subject: [PATCH 07/15] 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 6f94614..6c2bc3a 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 <andrew@system76.com>
Date: Fri, 23 Feb 2024 15:56:49 -0500
Subject: [PATCH 08/15] (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 6c2bc3a..e01bad6 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 <andrew@system76.com>
Date: Fri, 22 Mar 2024 15:10:15 -0400
Subject: [PATCH 09/15] 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 e01bad6..7f1bcfd 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 349dfae..4806d1f 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 67ed6fb..e7f74fd 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 0000000..9896378
--- /dev/null
+++ b/lib/recognizer_web/templates/accounts/user_settings/confirm_two_factor_external.html.eex
@@ -0,0 +1,26 @@
+<div class="box">
+    <h2 class="title is-2 mb-5 has-text-centered-mobile">Confirm Two Factor Authentication</h2>
+
+    <div class="content">
+        <p>If you did not receive a verification code, or it is expired, you can retry...(init)</p>
+    </div>
+
+    <div class="content mt-5">
+        <p>Enter the provided 6-digit code:</p>
+    </div>
+
+    <%= form_for @conn, Routes.user_settings_path(@conn, :two_factor_confirm), fn f -> %>
+    <div class="field">
+        <div class="control">
+            <%= 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)}" %>
+        </div>
+
+        <%= error_tag f, :two_factor_code %>
+    </div>
+
+    <div class="buttons is-right mt-6">
+        <%= submit "Verify Code", class: "button is-secondary" %>
+        <a href="/settings" class="button">Cancel</a>
+    </div>
+    <% end %>
+</div>

From 56a7430667274c4706658a45ffacbd22b19685b4 Mon Sep 17 00:00:00 2001
From: Andrew Hebert <andrew@system76.com>
Date: Fri, 22 Mar 2024 16:19:28 -0400
Subject: [PATCH 10/15] 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 7f1bcfd..62dbea8 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 9896378..f884ef4 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 @@
     <h2 class="title is-2 mb-5 has-text-centered-mobile">Confirm Two Factor Authentication</h2>
 
     <div class="content">
-        <p>If you did not receive a verification code, or it is expired, you can retry...(init)</p>
+        <p>If you did not receive a verification code, or it has expired, you can retry:</p>
+        <%= link "Send again", to: Routes.user_settings_path(@conn, :two_factor_init), class: "button is-secondary"%>
     </div>
 
     <div class="content mt-5">

From 8f292111fb4d0ac970aa460075d64701403ee669 Mon Sep 17 00:00:00 2001
From: Andrew Hebert <andrew@system76.com>
Date: Fri, 22 Mar 2024 17:11:39 -0400
Subject: [PATCH 11/15] 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 62dbea8..5cf6804 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 <andrew@system76.com>
Date: Mon, 25 Mar 2024 09:02:17 -0400
Subject: [PATCH 12/15] 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 f884ef4..4053e4e 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 @@
 <div class="box">
     <h2 class="title is-2 mb-5 has-text-centered-mobile">Confirm Two Factor Authentication</h2>
 
-    <div class="content">
-        <p>If you did not receive a verification code, or it has expired, you can retry:</p>
-        <%= link "Send again", to: Routes.user_settings_path(@conn, :two_factor_init), class: "button is-secondary"%>
-    </div>
-
     <div class="content mt-5">
         <p>Enter the provided 6-digit code:</p>
     </div>
@@ -23,5 +18,13 @@
         <%= submit "Verify Code", class: "button is-secondary" %>
         <a href="/settings" class="button">Cancel</a>
     </div>
+
+    <div class="content">
+        <p>If you did not receive a verification code, or it has expired:</p>
+        <div class="buttons is-right">
+            <%= link "Send another", to: Routes.user_settings_path(@conn, :two_factor_init), class: "button is-warning is-right"%>
+        </div>
+    </div>
+
     <% end %>
 </div>

From d4b13b2fa0e7f31a474e74c9e340564bdaded6eb Mon Sep 17 00:00:00 2001
From: Andrew Hebert <andrew@system76.com>
Date: Mon, 25 Mar 2024 11:52:47 -0400
Subject: [PATCH 13/15] 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 40c2707..7440906 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 @@
 
     <div class="buttons is-right mt-5">
         <div class="control">
-            <%= 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] %>
         </div>
     </div>
 

From b73456e63a5d52d80d6ed1dd97915f8b1477ee0b Mon Sep 17 00:00:00 2001
From: Andrew Hebert <andrew@system76.com>
Date: Mon, 25 Mar 2024 17:36:31 -0400
Subject: [PATCH 14/15] 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 5cf6804..b444a0b 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 e7f74fd..8e9d007 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 426b995..438db30 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 <andrew@system76.com>
Date: Tue, 26 Mar 2024 13:36:53 -0400
Subject: [PATCH 15/15] 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 76306af..23acf15 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 b444a0b..4ab222e 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 438db30..f00b491 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</h2>"
       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