Merge pull request #1188 from code-corps/security-improvements

Improve pwd reset security

Author: joshsmith

config/config.exs

@@ -86,6 +86,8 @@ config :code_corps, :sentry, CodeCorps.Sentry.Async
 
 config :code_corps, :processor, CodeCorps.Processor.Async
 
+config :code_corps, password_reset_timeout: 3600
+
 # Import environment specific config. This must remain at the bottom
 # of this file so it overrides the configuration defined above.
 import_config "#{Mix.env}.exs"

lib/code_corps/services/forgot_password.ex

@@ -8,7 +8,7 @@ defmodule CodeCorps.Services.ForgotPasswordService do
   """
   def forgot_password(email) do
     with %User{} = user <- Repo.get_by(User, email: email),
-        { :ok, %AuthToken{} = %{ value: token } } <- AuthToken.changeset(%AuthToken{}, user) |> Repo.insert
+      { :ok, %AuthToken{} = %{ value: token } } <- AuthToken.changeset(%AuthToken{}, user) |> Repo.insert
     do
       Emails.ForgotPasswordEmail.create(user, token) |> Mailer.deliver_now()
     else

lib/code_corps_web/controllers/password_controller.ex

@@ -9,6 +9,7 @@ defmodule CodeCorpsWeb.PasswordController do
   """
   def forgot_password(conn, %{"email" => email}) do
     ForgotPasswordService.forgot_password(email)
+    conn = Guardian.Plug.sign_out(conn, :default)
     conn |> put_status(:ok) |> render("show.json", email: email)
   end
 

lib/code_corps_web/controllers/password_reset_controller.ex

@@ -18,9 +18,10 @@ defmodule CodeCorpsWeb.PasswordResetController do
     - If no, a `422` response will return the error.
   """
   def reset_password(conn, %{"token" => reset_token, "password" => _password, "password_confirmation" => _password_confirmation} = params) do
-    with %AuthToken{user: user} <- AuthToken |> Repo.get_by(%{ value: reset_token }) |> Repo.preload(:user),
-         {:ok, _} <- Phoenix.Token.verify(CodeCorpsWeb.Endpoint, "user", reset_token, max_age: 3600),
+    with %AuthToken{user: user} = auth_token <- AuthToken |> Repo.get_by(%{ value: reset_token }) |> Repo.preload(:user),
+         {:ok, _} <- Phoenix.Token.verify(conn, "user", reset_token, max_age: Application.get_env(:code_corps, :password_reset_timeout)),
          {:ok, updated_user} <- user |> User.reset_password_changeset(params) |> Repo.update,
+         {:ok, _auth_token} <- auth_token |> Repo.delete,
          {:ok, auth_token, _claims} = updated_user |> Guardian.encode_and_sign(:token)
     do
       conn
@@ -29,6 +30,7 @@ defmodule CodeCorpsWeb.PasswordResetController do
       |> render("show.json", token: auth_token, user_id: updated_user.id, email: updated_user.email)
     else
       {:error, %Changeset{} = changeset} -> conn |> put_status(422) |> render(CodeCorpsWeb.ErrorView, :errors, data: changeset)
+      {:error, _} -> conn |> put_status(:not_found) |> render(CodeCorpsWeb.ErrorView, "404.json")
       nil -> conn |> put_status(:not_found) |> render(CodeCorpsWeb.ErrorView, "404.json")
     end
   end

test/lib/code_corps_web/controllers/password_controller_test.exs

@@ -6,7 +6,7 @@ defmodule CodeCorpsWeb.PasswordControllerTest do
 
   alias CodeCorps.AuthToken
 
-  test "creates and renders resource when email is valid", %{conn: conn} do
+  test "Unauthenticated - creates and renders resource when email is valid", %{conn: conn} do
     user = insert(:user)
     attrs = %{"email" => user.email}
     conn = post conn, password_path(conn, :forgot_password), attrs
@@ -18,6 +18,21 @@ defmodule CodeCorpsWeb.PasswordControllerTest do
     assert_delivered_email CodeCorps.Emails.ForgotPasswordEmail.create(user, token)
   end
 
+  @tag :authenticated
+  test "Authenticated - creates and renders resource when email is valid and removes session", %{conn: conn} do
+    user = insert(:user)
+    attrs = %{"email" => user.email}
+    conn = post conn, password_path(conn, :forgot_password), attrs
+    response = json_response(conn, 200)
+
+    assert response == %{ "email" => user.email }
+
+    %AuthToken{value: token} = Repo.get_by(AuthToken, user_id: user.id)
+    assert_delivered_email CodeCorps.Emails.ForgotPasswordEmail.create(user, token)
+
+    refute Guardian.Plug.authenticated?(conn)
+  end
+
   test "does not create resource and renders 200 when email is invalid", %{conn: conn} do
     user = insert(:user)
     attrs = %{"email" => "random_email@gmail.com"}

test/lib/code_corps_web/controllers/password_reset_controller_test.exs

@@ -2,17 +2,24 @@ defmodule CodeCorpsWeb.PasswordResetControllerTest do
   @moduledoc false
 
   use CodeCorpsWeb.ApiCase, resource_name: :password_reset
+  import CodeCorps.TestEnvironmentHelper
   alias CodeCorps.{User, AuthToken}
 
-  test "updates user password when data is valid", %{conn: conn} do
+  test "updates user password when data is valid and deletes auth token model", %{conn: conn} do
     current_user = insert(:user)
     {:ok, auth_token} = AuthToken.changeset(%AuthToken{}, current_user) |> Repo.insert
+
+    assert AuthToken |> Repo.get(auth_token.id) |> Map.get(:id) == auth_token.id
+
     attrs = %{"token" => auth_token.value, "password" => "123456", "password_confirmation" => "123456"}
     conn = post conn, password_reset_path(conn, :reset_password), attrs
     response = json_response(conn, 201)
+
     assert response
     encrypted_password = Repo.get(User, current_user.id).encrypted_password
+
     assert Comeonin.Bcrypt.checkpw("123456", encrypted_password)
+    assert AuthToken |> Repo.get(auth_token.id) == nil
   end
 
   test "does not create resource and renders errors when password does not match", %{conn: conn} do
@@ -21,6 +28,7 @@ defmodule CodeCorpsWeb.PasswordResetControllerTest do
     attrs = %{"token" => auth_token.value, "password" => "123456", "password_confirmation" => "another"}
     conn = post conn, password_reset_path(conn, :reset_password), attrs
     response = json_response(conn, 422)
+
     assert %{"errors" => [%{"detail" => "Password confirmation passwords do not match"}]} = response
   end
 
@@ -29,6 +37,17 @@ defmodule CodeCorpsWeb.PasswordResetControllerTest do
     {:ok, _} = AuthToken.changeset(%AuthToken{}, current_user) |> Repo.insert
     attrs = %{"token" => "random token", "password" => "123456", "password_confirmation" => "123456"}
     conn = post conn, password_reset_path(conn, :reset_password), attrs
+
+    assert json_response(conn, 404)
+  end
+
+  test "does not create resource and renders errors when error in token timeout occurs", %{conn: conn} do
+    modify_env(:code_corps, password_reset_timeout: 0)
+
+    current_user = insert(:user)
+    {:ok, auth_token} = AuthToken.changeset(%AuthToken{}, current_user) |> Repo.insert
+    attrs = %{"token" => auth_token.value, "password" => "123456", "password_confirmation" => "123456"}
+    conn = post conn, password_reset_path(conn, :reset_password), attrs
     assert json_response(conn, 404)
   end