Convert the login page to use controllers (#389)

* Fix bug in plug pipelines

This prevented the MethodOverride plug from working, which allows us to make links that have a `DELETE` method for example.

Also update the logout button to use delete.

* Add ability to clear token

* Add cancel button

* Start AuthenticationController

* [wip] first stage login checks phone validity

* [wip] add second show action

* Add cancelling

* Fix most tests

* Add tests; small refactor

* Test and refactor for cancel

* Remove dead code

---------

Co-authored-by: teesloane <[email protected]>

lib/bike_brigade/authentication_messenger.ex

@@ -29,6 +29,20 @@ defmodule BikeBrigade.AuthenticationMessenger do
     GenServer.call(pid, {:validate_token, phone, token_attempt})
   end
 
+  def clear_token(phone), do: clear_token(@name, phone)
+
+  def clear_token(pid, phone) do
+    GenServer.cast(pid, {:clear, phone})
+  end
+
+  if Mix.env() == :test do
+    def get_state(), do: get_state(@name)
+
+    def get_state(pid) do
+      GenServer.call(pid, :get_state)
+    end
+  end
+
   # Server (callbacks)
 
   @impl GenServer
@@ -53,7 +67,7 @@ defmodule BikeBrigade.AuthenticationMessenger do
 
   def handle_call({:validate_token, phone, token_attempt}, _from, state) do
     # TODO refactor
-    if !dev?() do
+    if dev?() do
       unless Map.has_key?(state, phone) do
         {:reply, {:error, :token_expired}, state}
       else
@@ -68,16 +82,29 @@ defmodule BikeBrigade.AuthenticationMessenger do
     end
   end
 
+  if Mix.env() == :test do
+    @impl GenServer
+    def handle_call(:get_state, _from, state) do
+      {:reply, state, state}
+    end
+  end
+
   @impl GenServer
   def handle_info({:expire, phone}, state) do
     {:noreply, Map.delete(state, phone)}
   end
 
+  @impl GenServer
+  def handle_cast({:clear, phone}, state) do
+    {:noreply, Map.delete(state, phone)}
+  end
+
   defp send_message(phone, token) do
     msg = [
       from: Messaging.outbound_number(),
       to: phone,
-      body: "Your BikeBrigade access code is #{token}.\n\n@#{BikeBrigadeWeb.Endpoint.host()} ##{token}"
+      body:
+        "Your BikeBrigade access code is #{token}.\n\n@#{BikeBrigadeWeb.Endpoint.host()} ##{token}"
     ]
 
     SmsService.send_sms(msg)

lib/bike_brigade_web/controllers/authentication.ex → lib/bike_brigade_web/controllers/authentication_controller.ex

@@ -1,12 +1,64 @@
-defmodule BikeBrigadeWeb.Authentication do
+defmodule BikeBrigadeWeb.AuthenticationController do
   use BikeBrigadeWeb, :controller
 
   import Plug.Conn
 
   alias BikeBrigade.Accounts
   alias BikeBrigade.AuthenticationMessenger
 
-  def login(conn, %{"login" => %{"phone" => phone, "token_attempt" => token_attempt}}) do
+  defmodule Login do
+    use BikeBrigade.Schema
+    import Ecto.Changeset
+
+    alias BikeBrigade.EctoPhoneNumber
+
+    @primary_key false
+    embedded_schema do
+      field :phone, EctoPhoneNumber.Canadian
+      field :token_attempt, :string
+    end
+
+    def validate_phone(attrs) do
+      %Login{}
+      |> cast(attrs, [:phone])
+      |> validate_required([:phone])
+      |> validate_user_exists(:phone)
+      |> Ecto.Changeset.apply_action(:insert)
+    end
+
+    defp validate_user_exists(changeset, field) when is_atom(field) do
+      validate_change(changeset, field, fn _, phone ->
+        case Accounts.get_user_by_phone(phone) do
+          nil -> [{field, "We can't find your number. Have you signed up for Bike Brigade?"}]
+          _ -> []
+        end
+      end)
+    end
+  end
+
+  def show(conn, %{"login" => attrs}) do
+    case Login.validate_phone(attrs) do
+      {:ok, login} ->
+        changeset = Ecto.Changeset.change(login)
+
+        conn
+        |> render("show.html", state: :token, changeset: changeset, layout: false)
+
+      {:error, %Ecto.Changeset{} = changeset} ->
+        conn
+        |> render("show.html", state: :phone, changeset: changeset, layout: false)
+    end
+  end
+
+  def show(conn, _params) do
+    changeset = Ecto.Changeset.change(%Login{})
+
+    conn
+    |> render("show.html", state: :phone, changeset: changeset, layout: false)
+  end
+
+  def login(conn, %{"login" => %{"phone" => phone, "token_attempt" => token_attempt}})
+      when not is_nil(token_attempt) do
     case AuthenticationMessenger.validate_token(phone, token_attempt) do
       :ok ->
         user = Accounts.get_user_by_phone(phone)
@@ -24,10 +76,36 @@ defmodule BikeBrigadeWeb.Authentication do
       {:error, :token_invalid} ->
         conn
         |> put_flash(:error, "Access code is invalid. Please try again.")
-        |> redirect(to: ~p"/login?#{%{phone: phone}}")
+        |> redirect(to: ~p"/login?#{%{login: %{phone: phone}}}")
     end
   end
 
+  def login(conn, %{"login" => %{"phone" => phone}}) do
+    with {:ok, login} <- Login.validate_phone(%{"phone" => phone}),
+         :ok <- AuthenticationMessenger.generate_token(login.phone) do
+      changeset = Ecto.Changeset.change(login)
+
+      conn
+      |> render("show.html", state: :token, changeset: changeset, layout: false)
+    else
+      {:error, %Ecto.Changeset{} = changeset} ->
+        conn
+        |> render("show.html", state: :phone, changeset: changeset, layout: false)
+
+      {:error, err} ->
+        conn
+        |> put_flash(:error, err)
+        |> redirect(to: ~p"/login")
+    end
+  end
+
+  def cancel(conn, %{"phone" => phone}) do
+    AuthenticationMessenger.clear_token(phone)
+
+    conn
+    |> redirect(to: ~p"/login")
+  end
+
   @doc "Set the session token and live socket for the user"
   def do_login(conn, user) do
     conn

lib/bike_brigade_web/controllers/authentication_html.ex

@@ -0,0 +1,6 @@
+defmodule BikeBrigadeWeb.AuthenticationHTML do
+  use BikeBrigadeWeb, :html
+
+  embed_templates "authentication_html/*"
+
+end

lib/bike_brigade_web/live/login_live.html.heex → lib/bike_brigade_web/controllers/authentication_html/show.html.heex

@@ -1,3 +1,7 @@
+<.flash kind={:info} title="Success!" flash={@flash} />
+<.flash kind={:warn} title="Alert!" flash={@flash} />
+<.flash kind={:error} title="Error!" flash={@flash} />
+
 <div class="px-8 mx-auto max-w-7xl sm:px-6 lg:px-8">
   <div class="flex flex-col justify-center py-12 min-h-screen-safe bg-gray-50 sm:px-6 lg:px-8">
     <div class="sm:mx-auto sm:w-full sm:max-w-md">
@@ -18,7 +22,7 @@
           :if={@state == :phone}
           for={@changeset}
           id="login-form"
-          phx-submit="submit_phone"
+          action={~p"/login"}
         >
           <.input type="tel" field={{f, :phone}} label="Phone Number" />
 
@@ -78,6 +82,14 @@
             pattern="\d{6}"
           />
           <:actions>
+            <.button
+              color={:white}
+              class="w-full"
+              href={~p"/login?phone=#{@changeset.data.phone}"}
+              method="delete"
+            >
+              Cancel
+            </.button>
             <.button type="submit" class="w-full">Sign in</.button>
           </:actions>
         </.simple_form>

lib/bike_brigade_web/core_components.ex

@@ -37,7 +37,7 @@ defmodule BikeBrigadeWeb.CoreComponents do
     values: [:none, :small, :normal, :medium, :full]
 
   attr :class, :string, default: nil
-  attr :rest, :global, include: ~w(href patch navigate disabled replace)
+  attr :rest, :global, include: ~w(href patch navigate disabled replace method)
   slot :inner_block, required: true
 
   @button_base_classes [

lib/bike_brigade_web/router.ex

@@ -12,7 +12,7 @@ defmodule BikeBrigadeWeb.Router do
   def handle_errors(_conn, %{reason: %BikeBrigadeWeb.DeliveryExpiredError{}}), do: :ok
   def handle_errors(conn, err), do: super(conn, err)
 
-  import BikeBrigadeWeb.Authentication,
+  import BikeBrigadeWeb.AuthenticationController,
     only: [
       require_authenticated_user: 2,
       require_dispatcher: 2,
@@ -82,8 +82,9 @@ defmodule BikeBrigadeWeb.Router do
   scope "/", BikeBrigadeWeb do
     pipe_through [:browser, :redirect_if_user_is_authenticated]
 
-    live "/login", LoginLive, :index
-    post "/login", Authentication, :login
+    get "/login", AuthenticationController, :show
+    post "/login", AuthenticationController, :login
+    delete "/login", AuthenticationController, :cancel
   end
 
   # this scope is for authenticated users that aren't dispatchers (ie: riders).
@@ -101,7 +102,7 @@ defmodule BikeBrigadeWeb.Router do
       live "/leaderboard", StatsLive.Leaderboard, :show
     end
 
-    delete "/logout", Authentication, :logout
+    delete "/logout", AuthenticationController, :logout
   end
 
   scope "/", BikeBrigadeWeb do

test/bike_brigade_web/controllers/authentication_controller_test.exs

@@ -0,0 +1,66 @@
+defmodule BikeBrigadeWeb.AuthenticationControllerTest do
+  use BikeBrigadeWeb.ConnCase
+  alias BikeBrigade.{Accounts, AuthenticationMessenger}
+
+  describe "login" do
+    setup do
+      rider = fixture(:rider)
+      {:ok, user} = Accounts.create_user_for_rider(rider)
+
+      %{user: user}
+    end
+
+    test "shows the login page", %{conn: conn} do
+      conn = get(conn, ~p"/login")
+      assert html_response(conn, 200) =~ "Sign into your Bike Brigade account"
+    end
+
+    test "errors when you send an invalid  number", %{conn: conn} do
+      conn = post(conn, ~p"/login", %{login: %{phone: "5555555555555555"}})
+      assert html_response(conn, 200) =~ "phone number is not valid for Canada"
+    end
+
+    test "errors when you send a number that doesn't exist", %{conn: conn} do
+      conn = post(conn, ~p"/login", %{login: %{phone: "6475555555"}})
+
+      assert html_response(conn, 200) =~
+               "We can't find your number. Have you signed up for Bike Brigade?"
+    end
+
+    test "shows the token page when you give a valid number", %{conn: conn, user: user} do
+      conn = post(conn, ~p"/login", %{login: %{phone: user.phone}})
+      assert html_response(conn, 200) =~ "We sent an authentication code to your phone number"
+    end
+
+    test "errors when you send an invalid token", %{conn: conn, user: user} do
+      conn = post(conn, ~p"/login", %{login: %{phone: user.phone}})
+
+      token = Map.get(BikeBrigade.AuthenticationMessenger.get_state(), user.phone)
+      conn = post(conn, ~p"/login", %{login: %{phone: user.phone, token_attempt: "not a token"}})
+
+      assert redirected_to(conn) == ~p"/login?login[phone]=#{user.phone}"
+      conn = get(conn, ~p"/login?login[phone]=#{user.phone}")
+      assert html_response(conn, 200) =~ "Access code is invalid. Please try again."
+
+      # Token is not regenerated after error
+      assert Map.get(BikeBrigade.AuthenticationMessenger.get_state(), user.phone) == token
+    end
+
+    test "logs you in with valid token", %{conn: conn, user: user} do
+      conn = post(conn, ~p"/login", %{login: %{phone: user.phone}})
+      token = Map.get(BikeBrigade.AuthenticationMessenger.get_state(), user.phone)
+      conn = post(conn, ~p"/login", %{login: %{phone: user.phone, token_attempt: token}})
+      assert redirected_to(conn) == ~p"/"
+
+      conn = get(conn, ~p"/home")
+      assert html_response(conn, 200) =~ "Welcome!"
+    end
+
+    test "lets you cancel a login", %{conn: conn, user: user} do
+      conn = post(conn, ~p"/login", %{login: %{phone: user.phone}})
+      conn = delete(conn, ~p"/login", %{phone: user.phone})
+      assert redirected_to(conn) == ~p"/login"
+      assert Map.get(BikeBrigade.AuthenticationMessenger.get_state(), user.phone) == nil
+    end
+  end
+end