Merge pull request #201 from danschultzer/handle-user-id-nil-values

Make sure nil values doesn't throw exceptions for authentication and get_by

CHANGELOG.md

@@ -3,6 +3,9 @@
 ## v1.0.9 (TBA)
 
 * Removed call to `Pow.Ecto.Context.repo/1`
+* Fixed bug with exception raised in `Pow.Ecto.Schema.normalize_user_id_field_value/1` when calling `Pow.Ecto.Context.get_by/2` with a non binary user id
+* Fixed bug with exception raised in `Pow.Ecto.Schema.normalize_user_id_field_value/1` when calling `Pow.Ecto.Context.authenticate/2` with a non binary user id
+* `Pow.Ecto.Context.authenticate/2` now returns nil if user id or password is nil
 
 ## v1.0.8 (2019-05-24)
 

lib/pow/ecto/context.ex

@@ -89,6 +89,8 @@ defmodule Pow.Ecto.Context do
 
   User schema module and repo module will be fetched from the config. The user
   id field is fetched from the user schema module.
+
+  The method will return nil if either the user id or password is nil.
   """
   @spec authenticate(map(), Config.t()) :: user() | nil
   def authenticate(params, config) do
@@ -97,14 +99,19 @@ defmodule Pow.Ecto.Context do
     login_value   = params[Atom.to_string(user_id_field)]
     password      = params["password"]
 
+    do_authenticate(user_id_field, login_value, password, config)
+  end
+
+  defp do_authenticate(_user_id_field, nil, _password, _config), do: nil
+  defp do_authenticate(user_id_field, login_value, password, config) do
     [{user_id_field, login_value}]
     |> get_by(config)
-    |> maybe_verify_password(password)
+    |> verify_password(password)
   end
 
-  defp maybe_verify_password(nil, _password),
-    do: nil
-  defp maybe_verify_password(user, password) do
+  defp verify_password(nil, _password), do: nil
+  defp verify_password(_user, nil), do: nil
+  defp verify_password(user, password) do
     case user.__struct__.verify_password(user, password) do
       true -> user
       _    -> nil
@@ -169,7 +176,7 @@ defmodule Pow.Ecto.Context do
     user_id_field = user_mod.pow_user_id_field()
 
     Enum.map(clauses, fn
-      {^user_id_field, value} -> {user_id_field, Schema.normalize_user_id_field_value(value)}
+      {^user_id_field, value} when is_binary(value) -> {user_id_field, Schema.normalize_user_id_field_value(value)}
       any -> any
     end)
   end

test/pow/ecto/context_test.exs

@@ -55,6 +55,13 @@ defmodule Pow.Ecto.ContextTest do
       assert Context.authenticate(%{"username" => "JOHN.doE", "password" => @password}, @username_config) == username_user
     end
 
+    test "handles nil values" do
+      refute Context.authenticate(%{"password" => @password}, @config)
+      refute Context.authenticate(%{"email" => nil, "password" => @password}, @config)
+      refute Context.authenticate(%{"email" => "test@example.com"}, @config)
+      refute Context.authenticate(%{"email" => "test@example.com", "password" => nil}, @config)
+    end
+
     test "authenticates with extra trailing and leading whitespace for user id field", %{user: user, username_user: username_user} do
       assert Context.authenticate(%{"email" => " test@example.com ", "password" => @password}, @config) == user
       assert Context.authenticate(%{"username" => " john.doe ", "password" => @password}, @username_config) == username_user
@@ -173,6 +180,12 @@ defmodule Pow.Ecto.ContextTest do
       assert get_by_user.id == username_user.id
     end
 
+    test "handles nil value before normalization of user id field value" do
+      assert_raise ArgumentError, ~r/Comparison with nil is forbidden as it is unsafe/, fn ->
+        Context.get_by([email: nil], @config)
+      end
+    end
+
     test "as `use Pow.Ecto.Context`", %{user: user} do
       get_by_user = Users.get_by(email: @email)
       assert get_by_user.id == user.id