ueberauth / guardian

Elixir Authentication
MIT License
3.43k stars 382 forks source link

Tutorial: Fixed major errors, minor errors. #602

Closed 7stud closed 4 years ago

7stud commented 5 years ago

closes #600

1) Because the tutorial says that you can use your own User module instead of doing:

$ mix phx.gen.context UserManager User users username:string password:string

it should be made explicit that if you do use your own User module, then you need to define certain functions for the tutorial to work properly.

2) In the file lib/auth_me/user_manager/user_manager.ex, the tutorial doesn't specify the required import statement to allow the query to work without error:

defmodule AuthMe.UserManager do
  ...
  ...
  def authenticate_user(username, plain_text_password) do
    query = from u in User, where: u.username == ^username
    ...
    ...

The required import statement is:

  import Ecto.Query, only: [from: 2]

3) The tutorial alternately uses "/secret" and "/protected" for the route to the protected page. The route needs to be the same everywhere in the tutorial.

4) As currently written, the function AuthMeWeb.SessionController.login_reply() (in the file lib/auth_me_web/controllers/session_controller.ex) will result in the error:

protocol Enumerable not implemented for %AuthMe.UserManager.User{meta:

Ecto.Schema.Metadata<:loaded, “users”>, id: 3, inserted_at: ~N[2019-07-15 05:56:22], password:

“$argon2id$v=19$m=131072,t=8,p=4$jinhp9gEDVENO2PIdm5zkg$ovideTURD59gCoqtE6UUuYZ/32Y MnpgppuRf0NFAJ/k”, updated_at: ~N[2019-07-15 05:56:22], username: “me”}. This protocol is implemented for: Ecto.Adapters.SQL.Stream, Postgrex.Stream, DBConnection.Stream, DBConnection.PrepareStream, HashSet, Range, Map, Function, List, Stream, Date.Range, HashDict, GenEvent.Stream, MapSet, File.Stream, IO.Stream

Here's the offending code:

defmodule AuthMeWeb.SessionController do
  ...
  alias AuthMe.{UserManager, UserManager.User, UserManager.Guardian}
  ...
  ...
  defp login_reply({:ok, user}, conn) do
    conn
    |> put_flash(:info, "Welcome back!")
    |> Guardian.Plug.sign_in(Guardian, user)
    |> redirect(to: "/secret")
  end

Due to the alias, the line:

Guardian.Plug.sign_in(Guardian, user)

is equivalent to:

AuthMe.UserManager.Guardian.Plug.sign_in(Guardian, user)

And, because a connection is piped in as the first argument, the call becomes:

AuthMe.UserManager.Guardian.Plug.sign_in(connection, Guardian, user)

If you refer to the Guardian.Plug.sign_in() docs , the arguments are correct. However, the definition of the function:

AuthMe.UserManager.Guardian.Plug.sign_in()

in the guardian source code is different than the definition in docs for the function:

Guardian.Plug.sign_in() 

How is that possible? The function AuthMe.UserManager.Guardian.Plug.sign_in() is injected into the file /lib/auth_me/user_manager/guardian.ex by the statement:

use Guardian, otp_app: :auth_me

which calls the Guardian.__using__() macro. The Guardian.__using__() macro is defined in the source code here and contains the line:

use Guardian.Plug, unquote(__MODULE__)

which calls the Guardian.Plug.__using__() macro. The Guardian.Plug.__using__() macro is defined in the source code here, and it contains the code:

quote do
    ...
    ...
    def sign_in(conn, resource, claims \\ %{}, opts \\ []),
          do: Guardian.Plug.sign_in(conn, implementation(), resource, claims, opts)

As can be see in that definition for sign_in(), the function should be called with two arguments: conn and the resource, i.e. user, not: conn, Guardian, user. When sign_in() is called with the latter arguments, user gets assigned to the parameter variable claims, and elsewhere in the guardian.ex source code the following executes:

      claims
      |> Enum.into(%{})

which causes the error: protocol Enumerable not implemented for %AuthMe.UserManager.User

4) A similar problem as in 3) exists for the logout() function in AuthMeWeb.SessionController.

5) At the end of the tutorial, I changed the description of how a user can verify that Guardian is actually working. The new description instructs the user to try to gain access to the "/protected" route without first logging in, as well as steps to experiment with the logout feature.

6) Fixed other minor errors.

codecov-io commented 5 years ago

Codecov Report

Merging #602 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #602   +/-   ##
=======================================
  Coverage   86.22%   86.22%           
=======================================
  Files          21       21           
  Lines         421      421           
=======================================
  Hits          363      363           
  Misses         58       58

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dfb7772...f71184f. Read the comment docs.

Hanspagh commented 5 years ago

@7stud any news on this?

Hanspagh commented 5 years ago

Hi @7stud. We love to have this change, how is it coming along?