Google authentication #24

SimonLab · 2019-11-21T10:47:19Z

ref: dwyl/app#247
Use elixir_auth_google to allow users to signin using OAuth2 Google.

SimonLab · 2019-11-21T10:56:55Z

Error on Travis due to a conflict of elixir versions. ~~I think refreshing the mix.lock should fix the error (or rebuilding the application)~~
Edit: I need to update the elixir version in travis.yml configuration file

nelsonic · 2019-11-26T19:02:20Z

priv/repo/migrations/20191126144556_create_session_table.exs

+      add :refresh_token, :string
+      add :key_id, :integer
+
+      add :person_id, references(:people, on_delete: :nothing)


nelsonic · 2019-11-27T11:39:07Z

priv/repo/migrations/20191127112248_remove_people_key_id.exs

+
+  def change do
+    alter table("people") do
+      remove :key_id


@SimonLab :key_id is for storing the id of the encryption key used to encrypt the data in the same row as the data so that it's clear which key to use to decrypt it.
Are you planning to store the encryption key id somewhere else? 💭

Yes the key_id has been added to the sessions table
see dwyl/app#247 (comment)

@SimonLab yeah, I saw that 👍
However I expect the data in these two tables will be encrypted with different encryption keys. If someone registers to use the App using just an email address and password dwyl/app#237 (a use-case we will 100% be adding to support people who don't have/use Google Accounts) Then we will need to have key_id in the people table to know which encryption key was used to encrypt their data on sign-up. On each subsequent login the session data (e.g: IP Address and Browser User Agent) will be encrypted and stored in the sessions table possibly with a different encryption key because of our key rotation system. 💭

Apologies that this is not clearly mapped out ... I don't expect you to guess what's in my head. 🙄

Yes makes sense, sorry I was focused on the google auth and hasn't thought of keeping the specific key_id for email/password authentication. I'll add it back 👍

Glad with the new solution for this. in-lining the key_id is more "user-friendly" (to devs) 👍

nelsonic · 2019-11-27T16:13:12Z

@SimonLab do you have time to do a couple of pomodoros of pairing on this?
I want to run the branch on my localhost but I don't want to step on your toes while you are working.
Are you working on fixing the build? https://travis-ci.com/dwyl/app-mvp-phoenix/builds/138393162#L307

SimonLab · 2019-11-27T16:28:15Z

I was a bit lost on how the associations works with Ecto so I'm working on this at the moment, ie creating people and sessions. I haven't looked at the failing build yet. I'll be on zoom

nelsonic · 2019-11-27T16:29:26Z

@SimonLab OK. happy to pair on any part of the work if you are.
LMK if you want to join the Zoom (standup) call earlier e.g: 16:40

nelsonic · 2019-11-27T18:47:26Z

lib/app/ctx.ex

+  def create_session(attrs \\ %{}) do
+    %Session{}
+    |> Session.changeset(attrs)
+    |> Ecto.build_ass


did you mean Ecto.build_assoc ? 💭

nelsonic · 2019-11-28T08:42:52Z

mix.exs

@@ -44,7 +44,8 @@ defmodule App.MixProject do
      {:jason, "~> 1.0"},
      {:plug_cowboy, "~> 2.0"},
      {:elixir_auth_google, git: "https://github.com/dwyl/elixir-auth-google.git", branch: "master" },
-      {:fields, git: "https://github.com/dwyl/fields.git", branch: "update-ecto"}
+      {:fields, path: "/home/simon/Documents/dwyl/fields"}


this won't work very well on CI ... 😜
(but I get it that you're testing ...) 👍

arf yes, well spotted, I'll add back the github link.
The Fields PR is now merged

@SimonLab thanks for merging the PR. 👍
I will add "enough" docs to fields to publish it to Hex.pm 📦
so we no longer have to use the GitHub link. 🙄

nelsonic · 2019-11-28T09:30:52Z

@SimonLab it appears we are "suffering" from not having fields published on Hex.pm ... 😞
If you can take a look at my PR: dwyl/fields#51 🔍 🙏
I will work on documenting fields this morning so that we can ship it. 📦

nelsonic · 2019-11-29T18:54:55Z

@SimonLab please ensure you have pushed your latest changes on this branch/PR so I can attempt to take a look at it over the weekend. (obvs you shouldn't be working on weekends, but I have to "catch up"...)
Thanks! ✨

SimonLab · 2019-12-11T08:57:31Z

I think this PR is ready for review now that the email/password authentication has been added. I've had a looked and removed unnecessary config and logs but otherwise it looks good to me.

nelsonic · 2019-12-11T09:10:12Z

config/config.exs

-  google_client_id: System.get_env("GOOGLE_CLIENT_ID"),
-  google_client_secret: System.get_env("GOOGLE_CLIENT_SECRET"),
-  google_scope: "profile email",
-  google_redirect_uri: "http://localhost:4000/google/auth/callback"


nelsonic · 2019-12-11T09:14:17Z

config/test.exs

@@ -16,3 +16,6 @@ config :app, AppWeb.Endpoint,

 # Print only warnings and errors during test
 config :logger, level: :warn
+
+# mock elixir_auth_google
+config :app, :elixir_auth_google, App.ElixirAuthGoogle.Mock


Is it worth documenting how this works somewhere?

nelsonic · 2019-12-11T09:16:23Z

lib/app/ctx.ex

-  def get_person!(id), do: Repo.get!(Person, id)
+  def get_person!(id) do
+    Repo.get!(Person, id)
+    # |> Repo.preload(sessions: :person)


If we don't need to preload the sessions, this function can be restored to def get_person!(id), do: Repo.get!(Person, id)
Not urgent.

nelsonic · 2019-12-11T09:25:08Z

lib/app/ctx.ex

@@ -581,4 +614,23 @@ defmodule App.Ctx do
  def change_timer(%Timer{} = timer) do
    Timer.changeset(timer, %{})
  end
+
+  alias App.Ctx.Session


I'm increasingly not a fan of the "Contexts" in Phoenix.
They don't help to organise code, they simply dump all code in to Ctx
that should be in the same file as the schema or controller.

Not a problem with this PR more something that I'm going to fix in a future refactor. 💭

nelsonic · 2019-12-11T14:19:05Z

lib/app_web/controllers/auth_controller.ex

+  end
+
+  def logout(conn) do
+    configure_session(conn, drop: true)


My reading of this function is that it will only drop the session for one browser tab.
If the user has multiple and only performs logout in one, will their session still be valid in the others?
OR will the cookie be destroyed for all tabs and thus invalidate the session?
(That still does not mitigate against a cookie replay though...) 💭

My understanding is that the function will drop the session for the browser (ie all the tabs). I'll double check this on my localhost.

nelsonic · 2019-12-11T14:24:57Z

lib/app_web/controllers/auth_controller.ex

+      person = person_id && App.Ctx.get_person!(person_id) ->
+        assign(conn, :current_person, person)
+
+      true ->


true catch all assigns :current_person as nil i.e. not authenticated. 👍

nelsonic · 2019-12-11T14:36:36Z

lib/app_web/controllers/page_controller.ex

+          create_basic_session(conn, person)
+
+        {:error, %Ecto.Changeset{} = changeset} ->
+          url_oauth_google = ElixirAuthGoogle.generate_oauth_url(conn)


this looks very similar to the index function above. could we just invoke that?

If we redirect to the index page we will loose the errors information contained in the changeset and the error won't be displayed to the user

nelsonic · 2019-12-11T14:44:48Z

lib/app_web/templates/page/index.html.eex

+<section class="tc">
+	<%= form_for @changeset, Routes.page_path(@conn, :register), fn f -> %>
+	<div class="pa2">
+		<%= text_input f, :email, placeholder: "email" %>


Should be <input type="email" ... > to allow browser to auto-complete email address.
https://hexdocs.pm/phoenix_html/Phoenix.HTML.Form.html#email_input/3

lib/app_web/templates/page/index.html.eex

Co-Authored-By: Nelson <nelson+github@dwyl.io>

nelsonic · 2019-12-11T15:06:31Z

lib/app_web/templates/person/form.html.eex

@@ -17,10 +17,6 @@
  <%= text_input f, :email %>
  <%= error_tag f, :email %>

-  <%= label f, :email_hash %>


good catch. thanks for removing. 👍

nelsonic · 2019-12-11T15:14:55Z

on my localhost I ran the following commands:

git pull 
# branch google-authentication-#247
mix deps.get
mix ecto.migrate
mix phx.server

Server runs fine:

But when I attempt to login using Google I see the error:

When I run the following command to reset the DB:

mix ecto.reset
mix phx.server

I see the following error:

[debug] ** (Ecto.NoResultsError) expected at least one result but got none in query:

from p0 in App.Ctx.Person,
  where: p0.id == ^1

    (ecto) lib/ecto/repo/queryable.ex:107: Ecto.Repo.Queryable.one!/3
    (app) lib/app_web/controllers/auth_controller.ex:15: AppWeb.Auth.call/2
    (app) AppWeb.Router.browser/2
    (app) lib/app_web/router.ex:1: AppWeb.Router.__pipe_through0__/1
    (phoenix) lib/phoenix/router.ex:283: Phoenix.Router.__call__/2
    (app) lib/app_web/endpoint.ex:1: AppWeb.Endpoint.plug_builder_call/2
    (app) lib/plug/debugger.ex:122: AppWeb.Endpoint."call (overridable 3)"/2
    (app) lib/app_web/endpoint.ex:1: AppWeb.Endpoint.call/2
    (phoenix) lib/phoenix/endpoint/cowboy2_handler.ex:42: Phoenix.Endpoint.Cowboy2Handler.init/4
    (cowboy) /Users/n/code/app-mvp-phoenix/deps/cowboy/src/cowboy_handler.erl:41: :cowboy_handler.execute/2
    (cowboy) /Users/n/code/app-mvp-phoenix/deps/cowboy/src/cowboy_stream_h.erl:320: :cowboy_stream_h.execute/3
    (cowboy) /Users/n/code/app-mvp-phoenix/deps/cowboy/src/cowboy_stream_h.erl:302: :cowboy_stream_h.request_process/3
    (stdlib) proc_lib.erl:249: :proc_lib.init_p_do_apply/3

Apparently invoking App.Ctx.get_person!(person_id) on controllers/auth_controller.ex:15
fails catastrophically if there are zero people in the DB ... 😞

@SimonLab want to pair on resolving this?

lib/app_web/controllers/auth_controller.ex

SimonLab · 2019-12-11T15:18:38Z

Yes I had the same issue on localhost. When you reset your database you will also need to delete the cookie session manually. I think you must have manage to login and your session still exists but now the database doesn't contains the user. This case shouldn't happen in "real life" but we should still had a condition to check for it

nelsonic · 2019-12-11T15:28:21Z

lib/app/ctx/person.ex

+
+  ## Example
+
+    iex> transform_profile_data_to_person(%{


need to update this example to use :atoms as keys in the Map.
Not necessary for this PR. but good to do in next iteration. 💭

nelsonic · 2019-12-11T15:31:03Z

lib/app/ctx/person.ex

+    |> Map.put(:picture, profile.picture)
+  end
+
+  @doc false


we need to avoid @doc false wherever possible. 🚫
It's the equivalent of this road (pointless) sign:

nelsonic

@SimonLab PR looks good, thanks! 👍

SimonLab added 2 commits November 19, 2019 12:03

add configuration to use elixir_auth_google, #247

0f20ef9

add elixir_auth_google to application, #247

67b3afd

SimonLab added the in-progress An issue or pull request that is being worked on by the assigned person label Nov 21, 2019

SimonLab self-assigned this Nov 21, 2019

SimonLab mentioned this pull request Nov 21, 2019

add configuration to use elixir_auth_google #23

Closed

update mix dependencies, dwyl/app#247

39d6a73

nelsonic temporarily deployed to production-google-authe-oif4on November 21, 2019 11:00 Inactive

update travis configuration to user elixir 1.9, #24 (comment)

0e56d9d

nelsonic temporarily deployed to production-google-authe-oif4on November 21, 2019 11:03 Inactive

SimonLab added 2 commits November 26, 2019 14:52

create sessions table, #247

93dae3b

add schema for session, #247

f2a9560

nelsonic reviewed Nov 26, 2019

View reviewed changes

remove key_id from people, #247

f8bb841

nelsonic reviewed Nov 27, 2019

View reviewed changes

SimonLab added 3 commits November 27, 2019 16:37

create new people on authentication, #247

d0e937c

add association people sessions, #247

2501c05

test inserted encrypted token in session table, #247

a4ac4f6

nelsonic reviewed Nov 27, 2019

View reviewed changes

use binary type to save tokens in Postgres, #247

9e1bf7d

nelsonic reviewed Nov 28, 2019

View reviewed changes

use published (alpha) version of elixir_auth_google in mix.exs

d0d7d8d

SimonLab added 2 commits December 11, 2019 08:40

remove unnecessary configuration for elixir_auth_google, #247

c740b63

remove IO.inspect, #247

9f6467d

SimonLab assigned nelsonic and unassigned SimonLab Dec 11, 2019

SimonLab added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Dec 11, 2019

nelsonic reviewed Dec 11, 2019

View reviewed changes

nelsonic added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Dec 11, 2019

nelsonic reviewed Dec 11, 2019

View reviewed changes

lib/app_web/templates/page/index.html.eex Outdated Show resolved Hide resolved

Update lib/app_web/templates/page/index.html.eex

50391a4

Co-Authored-By: Nelson <nelson+github@dwyl.io>

nelsonic reviewed Dec 11, 2019

View reviewed changes

lib/app_web/controllers/auth_controller.ex Show resolved Hide resolved

nelsonic reviewed Dec 11, 2019

View reviewed changes

nelsonic approved these changes Dec 11, 2019

View reviewed changes

nelsonic merged commit bd284db into master Dec 11, 2019

nelsonic deleted the google-authentication-#247 branch December 11, 2019 15:43

SimonLab mentioned this pull request Dec 11, 2019

Google auth review suggestions #43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google authentication #24

Google authentication #24

SimonLab commented Nov 21, 2019

SimonLab commented Nov 21, 2019 •

edited

Loading

nelsonic Nov 26, 2019

nelsonic Nov 27, 2019

SimonLab Nov 27, 2019

nelsonic Nov 27, 2019 •

edited

Loading

SimonLab Nov 27, 2019

nelsonic Dec 6, 2019

nelsonic commented Nov 27, 2019

SimonLab commented Nov 27, 2019 •

edited

Loading

nelsonic commented Nov 27, 2019

nelsonic Nov 27, 2019

nelsonic Nov 28, 2019

SimonLab Nov 28, 2019

nelsonic Nov 28, 2019

nelsonic commented Nov 28, 2019

nelsonic commented Nov 29, 2019

SimonLab commented Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic Dec 11, 2019 •

edited

Loading

SimonLab Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic Dec 11, 2019

SimonLab Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic commented Dec 11, 2019

SimonLab commented Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic Dec 11, 2019

nelsonic left a comment

Google authentication #24

Google authentication #24

Conversation

SimonLab commented Nov 21, 2019

SimonLab commented Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonic Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonic commented Nov 27, 2019

SimonLab commented Nov 27, 2019 • edited Loading

nelsonic commented Nov 27, 2019

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

nelsonic commented Nov 28, 2019

nelsonic commented Nov 29, 2019

SimonLab commented Dec 11, 2019

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

nelsonic Dec 11, 2019 • edited Loading

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

nelsonic commented Dec 11, 2019

SimonLab commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonic left a comment

Choose a reason for hiding this comment

SimonLab commented Nov 21, 2019 •

edited

Loading

nelsonic Nov 27, 2019 •

edited

Loading

SimonLab commented Nov 27, 2019 •

edited

Loading

nelsonic Dec 11, 2019 •

edited

Loading