Skip to content

Commit

Permalink
replace people.email (plaintext) with Fields.EmailEncrypted for priva…
Browse files Browse the repository at this point in the history
…cy/security #285 also comment out all insecure/unused code - to be removed shortly
  • Loading branch information
nelsonic committed Mar 9, 2023
1 parent da0af7e commit 2bbba99
Show file tree
Hide file tree
Showing 14 changed files with 324 additions and 285 deletions.
78 changes: 39 additions & 39 deletions lib/auth/accounts.ex
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ defmodule Auth.Accounts do
"""
def get_person_by_email(email) when is_binary(email) do
Repo.get_by(Person, email: email)
Repo.get_by(Person, email_hash: email)
end

@doc """
Expand All @@ -40,7 +40,7 @@ defmodule Auth.Accounts do
"""
def get_person_by_email_and_password(email, password)
when is_binary(email) and is_binary(password) do
person = Repo.get_by(Person, email: email)
person = Repo.get_by(Person, email_hash: email)
if Person.valid_password?(person, password), do: person
end

Expand Down Expand Up @@ -267,27 +267,27 @@ defmodule Auth.Accounts do
end
end

@doc """
Confirms a person by the given token.
If the token matches, the person account is marked as confirmed
and the token is deleted.
"""
def confirm_person(token) do
with {:ok, query} <- PersonToken.verify_email_token_query(token, "confirm"),
%Person{} = person <- Repo.one(query),
{:ok, %{person: person}} <- Repo.transaction(confirm_person_multi(person)) do
{:ok, person}
else
_ -> :error
end
end

defp confirm_person_multi(person) do
Ecto.Multi.new()
|> Ecto.Multi.update(:person, Person.confirm_changeset(person))
|> Ecto.Multi.delete_all(:tokens, PersonToken.person_and_contexts_query(person, ["confirm"]))
end
# @doc """
# Confirms a person by the given token.

# If the token matches, the person account is marked as confirmed
# and the token is deleted.
# """
# def confirm_person(token) do
# with {:ok, query} <- PersonToken.verify_email_token_query(token, "confirm"),
# %Person{} = person <- Repo.one(query),
# {:ok, %{person: person}} <- Repo.transaction(confirm_person_multi(person)) do
# {:ok, person}
# else
# _ -> :error
# end
# end

# defp confirm_person_multi(person) do
# Ecto.Multi.new()
# |> Ecto.Multi.update(:person, Person.confirm_changeset(person))
# |> Ecto.Multi.delete_all(:tokens, PersonToken.person_and_contexts_query(person, ["confirm"]))
# end

## Reset password

Expand All @@ -307,26 +307,26 @@ defmodule Auth.Accounts do
PersonNotifier.deliver_reset_password_instructions(person, reset_password_url_fun.(encoded_token))
end

@doc """
Gets the person by reset password token.
# @doc """
# Gets the person by reset password token.

## Examples
# ## Examples

iex> get_person_by_reset_password_token("validtoken")
%Person{}
# iex> get_person_by_reset_password_token("validtoken")
# %Person{}

iex> get_person_by_reset_password_token("invalidtoken")
nil
# iex> get_person_by_reset_password_token("invalidtoken")
# nil

"""
def get_person_by_reset_password_token(token) do
with {:ok, query} <- PersonToken.verify_email_token_query(token, "reset_password"),
%Person{} = person <- Repo.one(query) do
person
else
_ -> nil
end
end
# """
# def get_person_by_reset_password_token(token) do
# with {:ok, query} <- PersonToken.verify_email_token_query(token, "reset_password"),
# %Person{} = person <- Repo.one(query) do
# person
# else
# _ -> nil
# end
# end

@doc """
Resets the person password.
Expand Down
21 changes: 17 additions & 4 deletions lib/auth/accounts/person.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ defmodule Auth.Accounts.Person do
import Ecto.Changeset

schema "people" do
field :email, :string
field :confirmed_at, :naive_datetime
field :email, Fields.EmailEncrypted
field :email_hash, Fields.EmailHash
field :password, :string, virtual: true, redact: true
field :hashed_password, :string, redact: true
field :confirmed_at, :naive_datetime

timestamps()
end
Expand Down Expand Up @@ -41,11 +42,23 @@ defmodule Auth.Accounts.Person do
|> validate_password(opts)
end

defp put_email_hash(changeset) when not is_nil(changeset.changes.email) do
# dbg(changeset)
put_change(changeset, :email_hash, String.downcase(changeset.changes.email))
end

defp put_email_hash(changeset) do
# dbg(changeset)
changeset
end


defp validate_email(changeset, opts) do
changeset
|> validate_required([:email])
|> validate_format(:email, ~r/^[^\s]+@[^\s]+$/, message: "must have the @ sign and no spaces")
|> validate_length(:email, max: 160)
|> put_email_hash()
|> maybe_validate_unique_email(opts)
end

Expand Down Expand Up @@ -80,8 +93,8 @@ defmodule Auth.Accounts.Person do
defp maybe_validate_unique_email(changeset, opts) do
if Keyword.get(opts, :validate_email, true) do
changeset
|> unsafe_validate_unique(:email, Auth.Repo)
|> unique_constraint(:email)
|> unsafe_validate_unique(:email_hash, Auth.Repo)
|> unique_constraint(:email_hash)
else
changeset
end
Expand Down
48 changes: 25 additions & 23 deletions lib/auth/accounts/person_token.ex
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ defmodule Auth.Accounts.PersonToken do
field :token, :binary
field :context, :string
field :sent_to, :string
# field :sent_to, Fields.EmailEncrypted
# field :email_hash, Fields.EmailHash
belongs_to :person, Auth.Accounts.Person

timestamps(updated_at: false)
Expand Down Expand Up @@ -107,29 +109,29 @@ defmodule Auth.Accounts.PersonToken do
for resetting the password. For verifying requests to change the email,
see `verify_change_email_token_query/2`.
"""
def verify_email_token_query(token, context) do
case Base.url_decode64(token, padding: false) do
{:ok, decoded_token} ->
hashed_token = :crypto.hash(@hash_algorithm, decoded_token)
days = days_for_context(context)

query =
from token in token_and_context_query(hashed_token, context),
join: person in assoc(token, :person),
where: token.inserted_at > ago(^days, "day") and token.sent_to == person.email,
select: person

{:ok, query}
# phx.gen.auth boilerplate code not yet covered by tests ...
# coveralls-ignore-start
:error ->
:error
# coveralls-ignore-stop
end
end

defp days_for_context("confirm"), do: @confirm_validity_in_days
defp days_for_context("reset_password"), do: @reset_password_validity_in_days
# def verify_email_token_query(token, context) do
# case Base.url_decode64(token, padding: false) do
# {:ok, decoded_token} ->
# hashed_token = :crypto.hash(@hash_algorithm, decoded_token)
# days = days_for_context(context)

# query =
# from token in token_and_context_query(hashed_token, context),
# join: person in assoc(token, :person),
# where: token.inserted_at > ago(^days, "day") and token.sent_to == person.email,
# select: person

# {:ok, query}
# # phx.gen.auth boilerplate code not yet covered by tests ...
# # coveralls-ignore-start
# :error ->
# :error
# # coveralls-ignore-stop
# end
# end

# defp days_for_context("confirm"), do: @confirm_validity_in_days
# defp days_for_context("reset_password"), do: @reset_password_validity_in_days

@doc """
Checks if the token is valid and returns its underlying lookup query.
Expand Down
46 changes: 23 additions & 23 deletions lib/auth_web/controllers/person_confirmation_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -30,27 +30,27 @@ defmodule AuthWeb.PersonConfirmationController do

# Do not log in the person after confirmation to avoid a
# leaked token giving the person access to the account.
def update(conn, %{"token" => token}) do
case Accounts.confirm_person(token) do
{:ok, _} ->
conn
|> put_flash(:info, "Person confirmed successfully.")
|> redirect(to: ~p"/")

:error ->
# If there is a current person and the account was already confirmed,
# then odds are that the confirmation link was already visited, either
# by some automation or by the person themselves, so we redirect without
# a warning message.
case conn.assigns do
%{current_person: %{confirmed_at: confirmed_at}} when not is_nil(confirmed_at) ->
redirect(conn, to: ~p"/")

%{} ->
conn
|> put_flash(:error, "Person confirmation link is invalid or it has expired.")
|> redirect(to: ~p"/")
end
end
end
# def update(conn, %{"token" => token}) do
# case Accounts.confirm_person(token) do
# {:ok, _} ->
# conn
# |> put_flash(:info, "Person confirmed successfully.")
# |> redirect(to: ~p"/")

# :error ->
# # If there is a current person and the account was already confirmed,
# # then odds are that the confirmation link was already visited, either
# # by some automation or by the person themselves, so we redirect without
# # a warning message.
# case conn.assigns do
# %{current_person: %{confirmed_at: confirmed_at}} when not is_nil(confirmed_at) ->
# redirect(conn, to: ~p"/")

# %{} ->
# conn
# |> put_flash(:error, "Person confirmation link is invalid or it has expired.")
# |> redirect(to: ~p"/")
# end
# end
# end
end
63 changes: 32 additions & 31 deletions lib/auth_web/controllers/person_reset_password_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule AuthWeb.PersonResetPasswordController do

alias Auth.Accounts

plug :get_person_by_reset_password_token when action in [:edit, :update]
# plug :get_person_by_reset_password_token when action in [:edit, :update]

def new(conn, _params) do
render(conn, :new)
Expand All @@ -13,6 +13,7 @@ defmodule AuthWeb.PersonResetPasswordController do
if person = Accounts.get_person_by_email(email) do
Accounts.deliver_person_reset_password_instructions(
person,
# ~p"/people/reset_password/"
&url(~p"/people/reset_password/#{&1}")
)
end
Expand All @@ -25,34 +26,34 @@ defmodule AuthWeb.PersonResetPasswordController do
|> redirect(to: ~p"/")
end

def edit(conn, _params) do
render(conn, :edit, changeset: Accounts.change_person_password(conn.assigns.person))
end

# Do not log in the person after reset password to avoid a
# leaked token giving the person access to the account.
def update(conn, %{"person" => person_params}) do
case Accounts.reset_person_password(conn.assigns.person, person_params) do
{:ok, _} ->
conn
|> put_flash(:info, "Password reset successfully.")
|> redirect(to: ~p"/people/log_in")

{:error, changeset} ->
render(conn, :edit, changeset: changeset)
end
end

defp get_person_by_reset_password_token(conn, _opts) do
%{"token" => token} = conn.params

if person = Accounts.get_person_by_reset_password_token(token) do
conn |> assign(:person, person) |> assign(:token, token)
else
conn
|> put_flash(:error, "Reset password link is invalid or it has expired.")
|> redirect(to: ~p"/")
|> halt()
end
end
# def edit(conn, _params) do
# render(conn, :edit, changeset: Accounts.change_person_password(conn.assigns.person))
# end

# # Do not log in the person after reset password to avoid a
# # leaked token giving the person access to the account.
# def update(conn, %{"person" => person_params}) do
# case Accounts.reset_person_password(conn.assigns.person, person_params) do
# {:ok, _} ->
# conn
# |> put_flash(:info, "Password reset successfully.")
# |> redirect(to: ~p"/people/log_in")

# {:error, changeset} ->
# render(conn, :edit, changeset: changeset)
# end
# end

# defp get_person_by_reset_password_token(conn, _opts) do
# %{"token" => token} = conn.params

# if person = Accounts.get_person_by_reset_password_token(token) do
# conn |> assign(:person, person) |> assign(:token, token)
# else
# conn
# |> put_flash(:error, "Reset password link is invalid or it has expired.")
# |> redirect(to: ~p"/")
# |> halt()
# end
# end
end
6 changes: 3 additions & 3 deletions lib/auth_web/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ defmodule AuthWeb.Router do
post "/people/log_in", PersonSessionController, :create
get "/people/reset_password", PersonResetPasswordController, :new
post "/people/reset_password", PersonResetPasswordController, :create
get "/people/reset_password/:token", PersonResetPasswordController, :edit
put "/people/reset_password/:token", PersonResetPasswordController, :update
# get "/people/reset_password/:token", PersonResetPasswordController, :edit
# put "/people/reset_password/:token", PersonResetPasswordController, :update
end

scope "/", AuthWeb do
Expand All @@ -76,6 +76,6 @@ defmodule AuthWeb.Router do
get "/people/confirm", PersonConfirmationController, :new
post "/people/confirm", PersonConfirmationController, :create
get "/people/confirm/:token", PersonConfirmationController, :edit
post "/people/confirm/:token", PersonConfirmationController, :update
# post "/people/confirm/:token", PersonConfirmationController, :update
end
end
6 changes: 5 additions & 1 deletion mix.exs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ defmodule Auth.MixProject do
def project do
[
app: :auth,
version: "0.1.0",
version: "2.0.0",
elixir: "~> 1.14",
elixirc_paths: elixirc_paths(Mix.env()),
start_permanent: Mix.env() == :prod,
Expand Down Expand Up @@ -67,6 +67,10 @@ defmodule Auth.MixProject do
{:jason, "~> 1.2"},
{:plug_cowboy, "~> 2.5"},

# Field Validation and Encryption: github.com/dwyl/fields
{:fields, "~> 2.10.3"},


# Check test coverage: github.com/parroty/excoveralls
{:excoveralls, "~> 0.14.3", only: :test},
]
Expand Down
Loading

0 comments on commit 2bbba99

Please sign in to comment.