refactor: Replace Addict with Guardian #126

pmrukot · 2017-10-06T16:54:25Z

Summary
Replaces Addict with Guardian. Basically changes whole authentication model.

Related issues
#39

Test plan
Manual + additional unit tests. Register and login are not integrated in Elm as a part of single page app.

TODO

NOTE(mrapacz|12-10-2017):

What still needs to be done before merging:

A proper review
Backend unit tests for registration and all the other stuff that appeared
Moving the keys to envs and replacing the hardcoded values with System.get_env

mrapacz · 2017-10-06T17:08:40Z

aion/config/dev.exs

+
+config :guardian, Guardian,
+  allowed_algos: ["HS512"],
+  secret_key: "@11H41LW35tT3x45",


Thats super experimental PR, please do not laugh 😠

pmrukot · 2017-10-08T09:54:07Z

Added TODO section for easier tracking of a PR

mrapacz · 2017-10-08T10:04:10Z

Note that this PR's main goal is to swap the libraries.

Adding logout is not an essential part of this PR, it may actually be easier to push it as a separate change (we were able to do without it so far, which means we should be able to do without it in this PR too).
Adding login/registration page is not essential (at least not both of them at the same time). So far we were able to use only registration page to log in and test that everything works correctly and that may be enough to verify that PR changes are alright. In a separate PR we could extend the registration page with the login feature. If you introduce two different pages right now then it will be much more complex to merge them into one that would offer both registration and login later.

pmrukot · 2017-10-08T20:52:22Z

I'll make login/register page as one view. Idea of this PR is to make it work just as it was before. I agree that logout is not necessary but our users need to be able to register/login just as before.

mrapacz · 2017-10-08T21:26:44Z

This PR should contain whatever changes are needed to prove that addict was removed totally and guardian was successfully introduced. From that point we can begin to work paralelly on issues connected to that - logout for example (users didn't have an option to do that so far).

…lm side

pmrukot · 2017-10-11T20:50:54Z

Everything is done, I think this is enough for first version. Im not sure about the case when the token expires but lets leave this case for now. Branch is also synchronised with master.

But I have another issues that are more important:

need to verify if config is fine / add configuration for production
need help with dialyzer
mix.test is screaming something like:

== Compilation error on file lib/aion/endpoint.ex ==
** (UndefinedFunctionError) function Aion.Router.init/1 is undefined (module Aion.Router is not available)
    Aion.Router.init([])
    (plug) lib/plug/builder.ex:193: Plug.Builder.init_module_plug/3
    (plug) lib/plug/builder.ex:181: anonymous fn/4 in Plug.Builder.compile/3
    (elixir) lib/enum.ex:1755: Enum."-reduce/3-lists^foldl/2-0-"/3
    (plug) lib/plug/builder.ex:181: Plug.Builder.compile/3
    (phoenix) expanding macro: Phoenix.Endpoint.__before_compile__/1
    lib/aion/endpoint.ex:1: Aion.Endpoint (module)

So 2 important checks wont pass on CI (tests wont run and dialyzer also screams)

pmrukot · 2017-10-11T20:52:17Z

New login change:

New register page:

The rest is working as before. Can't wait to actually split this into two apps/repositories.

pmrukot · 2017-10-11T20:53:24Z

Start with manual tests, let's fix the CI and then start reviewing the code. Please checkout to this branch and if everything works for you as before.

mrapacz

Some of them are to-do-now issues, some of them are to be discussed. Fantastic job in general 👏 🔥 🎆 ✨

Too bad the PR is so big and the review takes so long 😆

mrapacz · 2017-10-13T10:40:59Z

aion/config/travis.exs

+  pool: Ecto.Adapters.SQL.Sandbox
+
+config :guardian, Guardian,


There should be one Guardian config only in config.exs, it should import a proper key from the environment (which will vary depending on the environment).

mrapacz · 2017-10-13T10:45:32Z

aion/mix.exs

-  ]
+     {:postgrex, ">= 0.0.0"},
+     {:simetric, "~> 0.1.0"},
+     {:exrm, "~> 1.0.8"},]


Move the bracket to a new line.

mrapacz · 2017-10-13T10:47:25Z

aion/test/controllers/answer_controller_test.exs

-    user = %{ email: "[email protected]", name: "something" }
-    conn = Plug.Test.init_test_session(build_conn(), %{current_user: user})
-    {:ok, conn: put_req_header(conn, "accept", "application/json")}
+    user = %Aion.User{ email: "[email protected]", name: "something", password: "2131231", id: 1 }


There should be no spaces after { and before }

mrapacz · 2017-10-13T10:47:57Z

aion/test/controllers/category_controller_test.exs

-    user = %{ email: "[email protected]", name: "something"}
-    conn = Plug.Test. init_test_session(build_conn(), %{current_user: user})
-    {:ok, conn: put_req_header(conn, "accept", "application/json")}
+    user = %Aion.User{ email: "[email protected]", name: "something", password: "2131231", id: 1 }


mrapacz · 2017-10-13T10:48:48Z

aion/test/controllers/room_controller_test.exs

-    user = %{ email: "[email protected]", name: "something" }
-    conn = Plug.Test. init_test_session(build_conn(), %{current_user: user})
-    {:ok, conn: put_req_header(conn, "accept", "application/json")}
+    user = %Aion.User{ email: "[email protected]", name: "something", password: "2131231", id: 1 }


{ samehere }

mrapacz · 2017-10-13T11:55:05Z

aion/web/elm/src/Update.elm

+    Forms.updateFormInput form name value
+
+
+unwrapToken : Maybe String -> String


How about

getToken maybeToken = case maybeToken of Just token -> token Nothing -> ""

mrapacz · 2017-10-13T11:57:03Z

aion/web/elm/src/Update.elm

+                oldDisplayLoginInsteadOfRegistration =
+                    oldAuthData.displayLoginInsteadOfRegistration
+            in
+                case oldDisplayLoginInsteadOfRegistration of


This will get hugely simplified when we introduce setters, so I'm not going to ask you to fix it now.

mrapacz · 2017-10-13T11:58:47Z

aion/web/elm/src/View.elm

+        includeNavbar =
+            case currentRoute of
+                AuthRoute ->
+                    \x y z -> x


Seriously, though, any idea for a better naming here? 😄

Plus if y and z are not needed then I suppose you could do \betterName _ _ -> betterName

mrapacz · 2017-10-13T12:02:25Z

aion/web/models/user.ex

-    |> validate_required([:name, :email, :encrypted_password])
+    |> cast(params, [:name, :email, :password])
+    |> validate_required([:name, :email, :password])
+    |> validate_length(:email, min: 3, max: 30)


We could move constants like this to a separate module or just out of this function and not hardcode them. (especially the latter one shouldn't be a lot of work, you could deal with that using the module @attributes which are conventional way of dealing with module constants in Elixir.

This is out of scope of this PR.

mrapacz · 2017-10-13T12:12:51Z

aion/web/views/user_view.ex

+  use Aion.Web, :view
+
+  def render("user.json", %{user: user}) do
+    if user != nil do


I'm still not sure about conventional way of dealing with nils, but this answer and Jose's answer suggest some solutions. I think the second one could actually be better (but it's just my personal opinion because I'm not a die hard fan of multiple function clauses). Here's how the first one could work:

def render("user.json", %user: user}) when is_nil user, do: %{} def render("user.json", %user: user}), do: Map.fetch(user, [:id, :name, :email])

Similarly to my suggestion ^ in the second one you should definitely use Map.fetch/2

Would this work?

def render("user.json", %{user: nil}), do: %{} def render("user.json", %{user: user}), do: Map.fetch(user, [:id, :name, :email])

Map.take/2*

yeah right, was looking at the docs and eventually used the wrong one
lgtm

mrapacz · 2017-10-13T12:25:47Z

aion/web/elm/src/Auth/Notifications.elm

@@ -16,4 +16,13 @@ registrationErrorToast =

 registrationSuccessfulToast : ( Model, Cmd Msg ) -> ( Model, Cmd Msg )
 registrationSuccessfulToast =
-    addToast (Toasty.Defaults.Success "Success!" "Account created successfuly - proceed to Login.")
+    addToast (Toasty.Defaults.Success "Success!" "Account created successfuly.")


successfully

jtkpiotr · 2017-10-13T17:59:28Z

aion/test/controllers/room_category_controller_test.exs

-    user = %{ email: "[email protected]", name: "something" }
-    conn = Plug.Test. init_test_session(build_conn(), %{current_user: user})
-    {:ok, conn: put_req_header(conn, "accept", "application/json")}
+    user = %Aion.User{ email: "[email protected]", name: "something", password: "2131231", id: 1 }


this piece of code repeats many times ;) How about extracting it somewhere ;)

jtkpiotr · 2017-10-13T18:06:53Z

aion/web/controllers/answer_controller.ex

+  def unauthenticated(conn, _params) do
+    conn
+    |> put_status(401)
+    |> render("error.json", message: "Authentication required")


I agree but I would prefer to put it in authorization related module

pmrukot · 2017-10-16T17:19:23Z

I extracted unauthenticated function to errors.ex in views. Test setup is also extracted. As for making a generic request function I would need to put a little more effort into it. Apart from that most of the changes are applied, let me know what you think.

mrapacz

Move the rest of the common errors to the error file. As for the rest of the issues, if something is not to be fixed during this PR just make sure there is an existing issue for that, so that we don't forget to fix it in the future (like the request stuff in Elm) 😃

mrapacz · 2017-10-16T17:24:33Z

aion/test/test_helper.exs

@@ -2,3 +2,18 @@ ExUnit.start

 Ecto.Adapters.SQL.Sandbox.mode(Aion.Repo, :manual)

+defmodule Aion.TestHelpers do


I guess the conventional way is to put it in the support/ and name it with _case:

mrapacz · 2017-10-16T17:25:01Z

aion/web/auth/auth.ex

+  def login_by_username_and_pass(conn, email, given_pass, opts) do
+    repo = Keyword.fetch!(opts, :repo)
+    user = repo.get_by(Aion.User, email: email)
+    cond do


Okay, let's leave it this way then

mrapacz · 2017-10-16T17:25:43Z

aion/web/controllers/registration_controller.ex

+        SessionController.create(conn, %{"email" => user.email, "password" => user.password})
+      {:error, changeset} ->
+        conn
+        |> put_status(:unprocessable_entity)


I thought this would also end up in errors.ex

mrapacz · 2017-10-16T17:26:47Z

aion/web/controllers/session_controller.ex

+        |> render("login.json", jwt: jwt)
+      {:error, _reason, conn} ->
+        conn
+        |> put_status(500)


mrapacz · 2017-10-16T17:28:03Z

aion/web/elm/src/Panel/Api.elm

+createQuestionWithAnswersRequest url token decoder body =
+    Http.request
+        { method = "POST"
+        , headers = [ Http.header "Authorization" ("Bearer " ++ token) ]


@pmrukot if it doesn't get fixed now, it should be added as a follow-up issue (you can add it to changes listed in #56)

mrapacz · 2017-10-16T17:30:35Z

aion/web/views/session_view.ex

+    %{"data": "failed to authorise"}
+  end
+
+  def render("login.json", %{jwt: jwt}) do


The jwt cannot be changed to token cause it would require a change in Guardian's implementation, right?

mrapacz · 2017-10-16T17:32:23Z

aion/web/views/errors.ex

+
+  def unauthenticated(conn) do
+    conn
+      |> put_status(401)


The tab here is redundant

mrapacz · 2017-10-16T17:37:11Z

aion/web/views/errors.ex

@@ -0,0 +1,12 @@
+defmodule Aion.ErrorCodesViews do


I think it would be easier if the module names resembled the file names, just like they did so far (e.g. RoomChannel -> room_channel.ex, UserRecord -> user_record.ex)

It would be even better if their whole name would represent the filepath, but it's not doable so easily with Phoenix 1.2 architecture. But the file names can mirror the module names at least, it's easier to search through the repo then.

Also, this is part of a controller, not of a view. I think putting it in the same directory as all the controllers would be alright (the code wouldn't move that far in the whole project directory). You could name it then like Aion.ControllerErrors or something similar

mrapacz · 2017-10-16T18:32:06Z

I've moved the errors stuff to a separate module in controllers directory. I think it looks better now. Also, the imports used in all of the controllers can be stored in the web.ex to avoid repeating stuff.

One last thing that needs to be done is moving Guardian logic to conn_case.ex

We've moved to a lib that is widely used so you shouldn't have any problems finding resources in this area like this or this

pmrukot · 2017-10-17T20:28:22Z

@mrapacz Issue with conn_case is resolved by adding another test helper since one of our tests need to have different headers to work properly.

I've also added tests for Aion.Auth module.

I think we are ready to merge if all issues are resolved.

mrapacz

So if I understand right the AuthConnCase sets up a logged in conn for the tests and ConnCase doesn't? Just small nits, looks good to me apart from that, good job 👍

mrapacz · 2017-10-18T17:36:06Z

aion/test/auth/auth_test.exs

+
+    login_result = Aion.Auth.login_by_username_and_pass(conn, @user.email, @user.password, repo: Aion.Repo)
+
+    assert tuple_size(login_result) == 2


Just for the future, I think in general we could match the exact results (if it's possible), cause just checking for list's/tuple's length may not be the most accurate thing.

This is what we've been doing so far, so no need to fix it, just suggesting that we could take another approach in the future.

I wanted to do it but it was not possible since authenticated conn contains things that depend on time (I think some kind of timestamp).

Second thing is that the result is a tuple that is {:ok, login(conn, user)} and login method was tested above. Also its the only result that provides tuple that has length 2 so I think its fine (although not that scalable).

mrapacz · 2017-10-18T17:37:08Z

aion/test/test_helper.exs

@@ -1,19 +1,3 @@
 ExUnit.start


I guess this file is not needed anymore?

It is

19:52:22.599 [error] Task #PID<0.314.0> started from #PID<0.73.0> terminating ** (stop) exited in: GenServer.call(ExUnit.Server, {:take_async_cases, 16}, 60000) ** (EXIT) no process: the process is not alive or there's no process currently associated with the given name, possibly because its application isn't started

mrapacz · 2017-10-18T17:43:17Z

Also, make sure you update the description of PR and its title/commit message. As for the latter one - pro tip:
github browser extensions
especially refined github which will set the commit message automatically as the PR title + issue number

pmrukot · 2017-10-18T17:56:01Z

AuthConnCase is used used in all controllers tests apart from PageControllerTest - it uses old ConnCase since it requires different headers to pass and renders html, not json as other controllers.

pmrukot added 2 commits September 14, 2017 19:18

fix: Remove addict

9ede0a0

Initial version

451d417

pmrukot added the Status: Work in progress label Oct 6, 2017

mrapacz reviewed Oct 6, 2017

View reviewed changes

pmrukot added 17 commits October 8, 2017 23:39

Make sockets auth work

c5973ea

Resolve get_user function ambiguity

76869a6

Prepare for Registration form

7ce9bcf

Initial register form

eb615ba

Working version of registration

fa851cc

Fetch resources after login

44b191a

Make local storage work

70af621

Try to init socket connection when app starts

0267611

Make /api/me work again, prototype of request with tokens

0847393

Make authenticated request to api/me

4162852

Delete unused code

257c7c0

Remove temp function

3a88577

Add logout

fc58275

Prevent errors on /api/me

98d1f9b

Protect sources from unauthorised request, add token to requests on E…

ef1a587

…lm side

Merge branch 'master' into refactor/replace-addict-with-guardian

d77d2ee

Make credo happy

c83aa0a

pmrukot added Status: Help Wanted! and removed Status: Work in progress labels Oct 11, 2017

pmrukot added 4 commits October 13, 2017 13:14

Don't hardcode url

4003044

Add notification when login fails

3a491e2

Remove wrong import

a273c89

Login user after register

c806c54

mrapacz requested changes Oct 13, 2017

View reviewed changes

Fix post auth actions

a82a276

mrapacz reviewed Oct 13, 2017

View reviewed changes

Move socket initialisation to Socket.elm

98fd3c1

jtkpiotr reviewed Oct 13, 2017

View reviewed changes

Review adjustments

febdfe7

mrapacz requested changes Oct 16, 2017

View reviewed changes

Extract controller errors

0d5758c

pmrukot added 5 commits October 17, 2017 21:32

Move setup to conn_case.ex

9373229

Make two separate ConnCase s

22061f5

Add login tests

dff20f6

Refactor login tests

b7c14d4

Add more test to authorization

8927b8f

mrapacz requested changes Oct 18, 2017

View reviewed changes

mrapacz approved these changes Oct 18, 2017

View reviewed changes

pmrukot merged commit f796ead into master Oct 18, 2017

pmrukot deleted the refactor/replace-addict-with-guardian branch October 18, 2017 19:27

This was referenced Oct 18, 2017

[Phoenix] Replace Addict with Guardian #39

Closed

enhancement(authentication): Easy movement between login and registration #111

Closed

enhancement(authentication): Logout #112

Closed

fix(elixir): Fix display of login page in firefox #113

Closed

		Forms.updateFormInput form name value


		unwrapToken : Maybe String -> String

		@@ -2,3 +2,18 @@ ExUnit.start

		Ecto.Adapters.SQL.Sandbox.mode(Aion.Repo, :manual)

		defmodule Aion.TestHelpers do


		login_result = Aion.Auth.login_by_username_and_pass(conn, @user.email, @user.password, repo: Aion.Repo)

		assert tuple_size(login_result) == 2

refactor: Replace Addict with Guardian #126

refactor: Replace Addict with Guardian #126

Conversation

pmrukot commented Oct 6, 2017 • edited Loading

NOTE(mrapacz|12-10-2017):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmrukot commented Oct 8, 2017

mrapacz commented Oct 8, 2017

pmrukot commented Oct 8, 2017

mrapacz commented Oct 8, 2017

pmrukot commented Oct 11, 2017

pmrukot commented Oct 11, 2017

pmrukot commented Oct 11, 2017

mrapacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmrukot commented Oct 16, 2017

mrapacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrapacz commented Oct 16, 2017

pmrukot commented Oct 17, 2017

mrapacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrapacz commented Oct 18, 2017

pmrukot commented Oct 18, 2017

pmrukot commented Oct 6, 2017 •

edited

Loading