Merge branch 'ah/bc-user-settings' into staging

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

@@ -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,6 @@ defmodule RecognizerWeb.Accounts.UserSettingsController do
       Accounts.get_new_two_factor_settings(user)
 
     if method == "text" || method == "voice" do
-      # TODO error without user phone
       :ok = Accounts.send_new_two_factor_notification(user, settings)
       render(conn, "confirm_two_factor_external.html")
     else
@@ -45,13 +45,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
 
@@ -77,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)
 
@@ -109,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)
 
@@ -119,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)
 

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

test/recognizer_web/controllers/accounts/user_settings_controller_test.exs

@@ -114,13 +114,41 @@ defmodule RecognizerWeb.Accounts.UserSettingsControllerTest do
       assert response =~ "Change Profile</h2>"
       assert response =~ "must not contain special characters"
     end
+
+    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
       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 +158,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 +204,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)