phone number required for text/voice 2fa

with flash error redirect

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
 

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)
 

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