BUG: New Person inserted each time I authenticate with Google

[nelsonic]

Minor issue: the App.Ctx.get_person_by_email invoked in google_auth_controller.ex
does not do what we expect it to ...
https://github.com/dwyl/app-mvp-phoenix/blob/5a9a0fddb2d908de48b2b4de9853dbcaef63a0e5/lib/app_web/controllers/google_auth_controller.ex#L15

This is never going to work in a world where the email field is encrypted ...
(i.e. each time the email address [email protected] is encrypted/inserted the ciphertext is different ...)
https://github.com/dwyl/app-mvp-phoenix/blob/5a9a0fddb2d908de48b2b4de9853dbcaef63a0e5/lib/app/ctx.ex#L238

The result is that a new person record is inserted each time the person uses Google Auth:

🙄

Todo

• Ensure that the people schema has field :email_hash, Fields.EmailHash defined
  • Confirm that all tests still pass after updating the field definition.
• modify the App.Ctx.get_person_by_email/1 function to:
  • Use Fields.EmailHash.dump(email) to hash the email address before attempting to look it up.
  • Lookup an email by the email_hash not the email (Fields.EmailEncrypted)

def get_person_by_email(email) do
  email_hash = Fields.EmailHash.dump(email)
  Repo.get_by(Person, email_hash: email_hash)
end 

[SimonLab]

Confirm that all tests still pass after updating the field definition.

No they don't!

Updating the tests now. Linked also to #25
The cast function from EmailHash is working properly and returned an error as the format of the test email is not valid:

errors: [email_hash: {"is invalid", [type: Fields.EmailHash, validation: :cast]}]

In fact the error is due to the wrong validation applied from EmailHash field, see dwyl/fields#62

So the errors are linked to endpoints created automatically (ex: creating new person). The form created was trying to display the email hashed however Phoenix wasn't able to display the hash correctly.
I've manage to reduce the number of failing tests (just 5 left to fix). However I feel that we can start to try cleaning the endpoints especially if we are going to use Elm soon

[SimonLab]

The function get_by_email still returns nil when search on email_hash:

SELECT p0."id", p0."email", p0."email_hash", p0."familyName", p0."givenName", p0."locale", p0."password_hash", p0."picture", p0."username", p0."username_hash", p0."status", p0."tag", p0."key_id", p0."inserted_at", p0."updated_at" FROM "people" AS p0 WHERE (p0."email_hash" = $1) [<<35, 170, 210, 164, 226, 27, 120, 16, 40, 188, 159, 44, 154, 226, 106, 172, 17, 237, 33, 235, 188, 25, 243, 86, 120, 63, 191, 122, 61, 154, 118, 198>>]

I think the value are different between the hash converted and the actual value inseted in the database

[SimonLab]

I've updated the way we save the email_hash to be able to retrieve the person by email.
Another error is displayed when the Person already exists in the database:

function App.Ctx.Person.fetch/2 is undefined (App.Ctx.Person does not implement the Access behaviour)

I think this is due to some person data not defined and required on the index template

[nelsonic]

Hmm... can you share your screen? I'm on Zoom.

[SimonLab]

The email attribute wasn't pattern matched on the correct parameters. It was done on the person instead of attrs

 def google_changeset(profile, %{"email" => email} = attrs) do
    profile
    |> cast(attrs, [:email, :givenName, :familyName, :picture, :locale])
    |> validate_required([:email])
    |> put_change(:email_hash, email )
  end

This works however I'm wondering if there is a nicer/other way to extract the email directly from the changeset after |> validate_required([:email]). I can create a private function which add the hashed email similar to the one used to define the hashed password:

defp put_pass_hash(changeset) do
case changeset do
%Ecto.Changeset{valid?: true, changes: %{password: pass}} ->
put_change(changeset, :password_hash, Comeonin.Pbkdf2.hashpwsalt(pass))
_ ->
changeset
end
end

[SimonLab]

When a person already exists in the database we are only creating a new session then rendering the

next error to resolve:

[SimonLab]

Schemas don't have the Access Behaviour, so we can't access data with [ ], ie people["email"] returns an error. Using . is fine, people.email will work.

[nelsonic]

@SimonLab good catch. thanks for fixing this. 👍

[nelsonic]

Using auth_plug + auth this is no longer an issue. #65