diff --git a/lib/demo_web/templates/user_registration/new.html.eex b/lib/demo_web/templates/user_registration/new.html.eex index 2307f36..ee93159 100644 --- a/lib/demo_web/templates/user_registration/new.html.eex +++ b/lib/demo_web/templates/user_registration/new.html.eex @@ -8,11 +8,11 @@ <% end %> <%= label f, :email %> - <%= text_input f, :email %> + <%= text_input f, :email, required: true %> <%= error_tag f, :email %> <%= label f, :password %> - <%= password_input f, :password %> + <%= password_input f, :password, required: true %> <%= error_tag f, :password %>
diff --git a/lib/demo_web/templates/user_session/new.html.eex b/lib/demo_web/templates/user_session/new.html.eex new file mode 100644 index 0000000..fa8ae92 --- /dev/null +++ b/lib/demo_web/templates/user_session/new.html.eex @@ -0,0 +1,19 @@ +

Login

+ +<%= form_for :user, Routes.user_session_path(@conn, :create), fn f -> %> + <%= if @error_message do %> +
+

<%= @error_message %>

+
+ <% end %> + + <%= label f, :email %> + <%= text_input f, :email, required: true %> + + <%= label f, :password %> + <%= password_input f, :password, required: true %> + +
+ <%= submit "Login" %> +
+<% end %> diff --git a/lib/demo_web/views/user_session_view.ex b/lib/demo_web/views/user_session_view.ex new file mode 100644 index 0000000..6708b42 --- /dev/null +++ b/lib/demo_web/views/user_session_view.ex @@ -0,0 +1,3 @@ +defmodule DemoWeb.UserSessionView do + use DemoWeb, :view +end From eff837696aaba28200566ac74e4426de2218fdda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 17 Mar 2020 17:25:21 +0100 Subject: [PATCH 03/76] Add user confirmation --- lib/demo/accounts.ex | 66 ++++++++++++++++++- lib/demo/accounts/user.ex | 9 +++ lib/demo/accounts/user_notifier.ex | 24 +++++++ lib/demo/accounts/user_token.ex | 55 ++++++++++++++++ lib/demo_web/controllers/user_confirmation.ex | 43 ++++++++++++ .../user_registration_controller.ex | 6 ++ lib/demo_web/router.ex | 8 +++ .../templates/layout/_user_menu.html.eex | 4 +- .../templates/user_confirmation/new.html.eex | 10 +++ lib/demo_web/views/user_confirmation_view.ex | 3 + .../20200316103722_create_auth_tables.exs | 15 ++++- 11 files changed, 235 insertions(+), 8 deletions(-) create mode 100644 lib/demo/accounts/user_notifier.ex create mode 100644 lib/demo/accounts/user_token.ex create mode 100644 lib/demo_web/controllers/user_confirmation.ex create mode 100644 lib/demo_web/templates/user_confirmation/new.html.eex create mode 100644 lib/demo_web/views/user_confirmation_view.ex diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 4c8b89f..78118e8 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -3,10 +3,8 @@ defmodule Demo.Accounts do The Accounts context. """ - import Ecto.Query, warn: false alias Demo.Repo - - alias Demo.Accounts.User + alias Demo.Accounts.{User, UserToken, UserNotifier} @doc """ Gets a single user. @@ -22,6 +20,22 @@ defmodule Demo.Accounts do """ def get_user(id), do: Repo.get(User, id) + @doc """ + Gets a user by email. + + ## Examples + + iex> get_user_by_email("foo@example.com") + %User{} + + iex> get_user_by_emai("unknown@example.com") + nil + + """ + def get_user_by_email(email) when is_binary(email) do + Repo.get_by(User, email: email) + end + @doc """ Gets a user by email and password. @@ -86,4 +100,50 @@ defmodule Demo.Accounts do def registration_user(%User{} = user, attrs \\ %{}) do User.registration_changeset(user, attrs) end + + @doc """ + Delivers the confirmation e-mail instructions to the given user. + + ## Examples + + iex> deliver_confirmation_instructions(user, &Routes.user_confirmation_url(conn, :confirm)) + :ok + + iex> deliver_confirmation_instructions(confirmed_user, &Routes.user_confirmation_url(conn, :confirm)) + {:error, :already_confirmed} + + """ + def deliver_confirmation_instructions(%User{} = user, confirmation_url_fun) + when is_function(confirmation_url_fun, 1) do + if user.confirmed_at do + {:error, :already_confirmed} + else + {encoded_token, user_token} = UserToken.confirmation_token(user) + Repo.insert!(user_token) + UserNotifier.deliver_confirmation_instructions(user, confirmation_url_fun.(encoded_token)) + :ok + end + end + + @doc """ + Confirms a user by the given token. + + If the token matches, the user account is marked as confirmed + and the token is deleted. + """ + def confirm_user(token) do + with {:ok, query} <- UserToken.confirmation_query(token), + {token, user} <- Repo.one(query), + {:ok, _} <- Repo.transaction(confirmation_multi(user, token)) do + :ok + else + _ -> :error + end + end + + defp confirmation_multi(user, token) do + Ecto.Multi.new() + |> Ecto.Multi.update(:user, User.confirm_changeset(user)) + |> Ecto.Multi.delete(:user_token, token) + end end diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index c9e79fe..2269199 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -7,6 +7,7 @@ defmodule Demo.Accounts.User do field :password, :string, virtual: true field :encrypted_password, :string field :confirmed_at, :naive_datetime + has_many :tokens, Demo.Accounts.UserToken, foreign_key: :user_id timestamps() end @@ -58,4 +59,12 @@ defmodule Demo.Accounts.User do Bcrypt.hash_pwd_salt("unused hash to avoid timing attacks") false end + + @doc """ + Confirms the account by setting `confirmed_at`. + """ + def confirm_changeset(user) do + now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second) + change(user, confirmed_at: now) + end end diff --git a/lib/demo/accounts/user_notifier.ex b/lib/demo/accounts/user_notifier.ex new file mode 100644 index 0000000..1dbbf20 --- /dev/null +++ b/lib/demo/accounts/user_notifier.ex @@ -0,0 +1,24 @@ +defmodule Demo.Accounts.UserNotifier do + # For simplicity, this module simply prints messages to the terminal. + # You should replace it by a proper e-mail or notification tool, such as: + # + # * Swoosh - https://github.com/swoosh/swoosh + # * Bamboo - https://github.com/thoughtbot/bamboo + # + def deliver_confirmation_instructions(user, url) do + IO.puts """ + + ============================== + + Hi #{user.email}, + + You can confirm your account by visiting the url below: + + #{url} + + If you didn't create an account with us, please ignore this. + + ============================== + """ + end +end diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex new file mode 100644 index 0000000..5e73100 --- /dev/null +++ b/lib/demo/accounts/user_token.ex @@ -0,0 +1,55 @@ +defmodule Demo.Accounts.UserToken do + use Ecto.Schema + import Ecto.Query + + @hash_algorithm :sha256 + @rand_size 32 + + schema "user_tokens" do + field :hashed_token, :binary + field :context, :string + field :sent_to, :string + belongs_to :user, Demo.Accounts.User + + timestamps(updated_at: false) + end + + def confirmation_token(user) do + emailable_token(user, "confirm", user.email) + end + + def confirmation_query(token) do + case Base.url_decode64(token, padding: false) do + {:ok, decoded_token} -> + hashed_token = :crypto.hash(@hash_algorithm, decoded_token) + + query = + from token in Demo.Accounts.UserToken, + join: user in assoc(token, :user), + where: + token.hashed_token == ^hashed_token and + token.context == "confirm" and + token.inserted_at > ago(1, "week") and + token.sent_to == user.email, + select: {token, user} + + {:ok, query} + + :error -> + :error + end + end + + defp emailable_token(user, context, sent_to) do + token = :crypto.strong_rand_bytes(@rand_size) + hashed_token = :crypto.hash(@hash_algorithm, token) + + {Base.url_encode64(token, padding: false), + %Demo.Accounts.UserToken{ + hashed_token: hashed_token, + context: context, + sent_to: sent_to, + user_id: user.id + }} + end +end diff --git a/lib/demo_web/controllers/user_confirmation.ex b/lib/demo_web/controllers/user_confirmation.ex new file mode 100644 index 0000000..dcff5b8 --- /dev/null +++ b/lib/demo_web/controllers/user_confirmation.ex @@ -0,0 +1,43 @@ +defmodule DemoWeb.UserConfirmationController do + use DemoWeb, :controller + + alias Demo.Accounts + + def new(conn, _params) do + render(conn, "new.html") + end + + def create(conn, %{"user" => %{"email" => email}}) do + if user = Accounts.get_user_by_email(email) do + Accounts.deliver_confirmation_instructions( + user, + &Routes.user_confirmation_url(conn, :confirm, &1) + ) + end + + # Regardless of the outcome, show an impartial success/error message. + conn + |> put_flash( + :info, + "If your e-mail is in our system and it has not been confirmed yet, " <> + "you will receive an e-mail with instructions shortly." + ) + |> redirect(to: "/") + end + + # Do not login the user after confirmation to avoid a + # leaked token giving the user access to the account. + def confirm(conn, %{"token" => token}) do + case Accounts.confirm_user(token) do + :ok -> + conn + |> put_flash(:info, "Account confirmed successfully.") + |> redirect(to: "/") + + :error -> + conn + |> put_flash(:error, "Confirmation token is invalid or it has expired.") + |> redirect(to: Routes.user_confirmation_path(conn, :new)) + end + end +end diff --git a/lib/demo_web/controllers/user_registration_controller.ex b/lib/demo_web/controllers/user_registration_controller.ex index 297f92f..124bea7 100644 --- a/lib/demo_web/controllers/user_registration_controller.ex +++ b/lib/demo_web/controllers/user_registration_controller.ex @@ -13,6 +13,12 @@ defmodule DemoWeb.UserRegistrationController do def create(conn, %{"user" => user_params}) do case Accounts.register_user(user_params) do {:ok, user} -> + :ok = + Accounts.deliver_confirmation_instructions( + user, + &Routes.user_confirmation_url(conn, :confirm, &1) + ) + conn |> put_flash(:info, "User created successfully.") |> UserAuth.login_user(user) diff --git a/lib/demo_web/router.ex b/lib/demo_web/router.ex index 54bf3cd..40333f3 100644 --- a/lib/demo_web/router.ex +++ b/lib/demo_web/router.ex @@ -44,4 +44,12 @@ defmodule DemoWeb.Router do delete "/users/logout", UserSessionController, :delete end + + scope "/", DemoWeb do + pipe_through [:browser] + + get "/users/confirm/new", UserConfirmationController, :new + post "/users/confirm", UserConfirmationController, :create + get "/users/confirm/:token", UserConfirmationController, :confirm + end end diff --git a/lib/demo_web/templates/layout/_user_menu.html.eex b/lib/demo_web/templates/layout/_user_menu.html.eex index 734ad1a..4bf844c 100644 --- a/lib/demo_web/templates/layout/_user_menu.html.eex +++ b/lib/demo_web/templates/layout/_user_menu.html.eex @@ -1,9 +1,9 @@
    <%= if @current_user do %>
  • <%= @current_user.email %>
    • -
  • <%= link "Sign out", to: Routes.user_session_path(@conn, :delete), method: :delete %>
    • +
  • <%= link "Logout", to: Routes.user_session_path(@conn, :delete), method: :delete %>
    • <% else %>
  • <%= link "Register", to: Routes.user_registration_path(@conn, :new) %>
    • -
  • <%= link "Sign in", to: Routes.user_session_path(@conn, :new) %>
    • +
  • <%= link "Login", to: Routes.user_session_path(@conn, :new) %>
    • <% end %>
\ No newline at end of file diff --git a/lib/demo_web/templates/user_confirmation/new.html.eex b/lib/demo_web/templates/user_confirmation/new.html.eex new file mode 100644 index 0000000..2f58f4d --- /dev/null +++ b/lib/demo_web/templates/user_confirmation/new.html.eex @@ -0,0 +1,10 @@ +

Resend confirmation instructions

+ +<%= form_for :user, Routes.user_confirmation_path(@conn, :create), fn f -> %> + <%= label f, :email %> + <%= text_input f, :email, required: true %> + +
+ <%= submit "Resend confirmation instructions" %> +
+<% end %> diff --git a/lib/demo_web/views/user_confirmation_view.ex b/lib/demo_web/views/user_confirmation_view.ex new file mode 100644 index 0000000..0d0bee6 --- /dev/null +++ b/lib/demo_web/views/user_confirmation_view.ex @@ -0,0 +1,3 @@ +defmodule DemoWeb.UserConfirmationView do + use DemoWeb, :view +end diff --git a/priv/repo/migrations/20200316103722_create_auth_tables.exs b/priv/repo/migrations/20200316103722_create_auth_tables.exs index d669814..8529031 100644 --- a/priv/repo/migrations/20200316103722_create_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_auth_tables.exs @@ -3,11 +3,20 @@ defmodule Demo.Repo.Migrations.CreateAuthTables do def change do create table(:users) do - add :email, :string - add :encrypted_password, :string + add :email, :string, null: false + add :encrypted_password, :string, null: false add :confirmed_at, :naive_datetime - timestamps() end + + create unique_index(:users, [:email]) + + create table(:user_tokens) do + add :user_id, references(:users, on_delete: :delete_all), null: false + add :hashed_token, :binary, null: false + add :context, :string, null: false + add :sent_to, :string + add :inserted_at, :naive_datetime + end end end From d517b5440d526654752a76170be5d4bcf96d5fb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 18 Mar 2020 00:15:52 +0100 Subject: [PATCH 04/76] Use tokens for login/logout --- README.md | 2 +- lib/demo/accounts.ex | 51 ++++++----- lib/demo/accounts/user.ex | 5 +- lib/demo/accounts/user_token.ex | 90 ++++++++++++++----- lib/demo_web/controllers/user_auth.ex | 26 +++++- lib/demo_web/controllers/user_confirmation.ex | 2 +- .../controllers/user_session_controller.ex | 4 +- .../templates/user_session/new.html.eex | 2 +- .../20200316103722_create_auth_tables.exs | 4 +- 9 files changed, 131 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index d7e5aa8..12767e6 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ We will two database tables: "users" and "users_tokens": * "users" will have the "encrypted_password", "email" and "confirmed_at" fields plus timestamps -* "users_tokens" will have "hashed_token", "context", "sent_to", "inserted_at" fields +* "users_tokens" will have "token", "context", "sent_to", "inserted_at" fields On sign in, we need to renew the session and delete the CSRF token. The password should be limited to 80 characters, the email to 160. diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 78118e8..96b47e1 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -6,20 +6,6 @@ defmodule Demo.Accounts do alias Demo.Repo alias Demo.Accounts.{User, UserToken, UserNotifier} - @doc """ - Gets a single user. - - ## Examples - - iex> get_user(123) - %User{} - - iex> get_user(456) - nil - - """ - def get_user(id), do: Repo.get(User, id) - @doc """ Gets a user by email. @@ -101,6 +87,31 @@ defmodule Demo.Accounts do User.registration_changeset(user, attrs) end + @doc """ + Generates a session/cookie token. + """ + def generate_to_be_signed_token(user, context) do + {token, user_token} = UserToken.build_to_be_signed_token(user, context) + Repo.insert!(user_token) + token + end + + @doc """ + Gets the user with the given signed token. + """ + def get_user_by_signed_token(token, context) do + {:ok, query} = UserToken.verify_to_be_signed_token_query(token, context) + Repo.one(query) + end + + @doc """ + Deletes the signed token with the given context. + """ + def delete_signed_token(token, context) do + Repo.delete_all(UserToken.token_and_context_query(token, context)) + :ok + end + @doc """ Delivers the confirmation e-mail instructions to the given user. @@ -118,7 +129,7 @@ defmodule Demo.Accounts do if user.confirmed_at do {:error, :already_confirmed} else - {encoded_token, user_token} = UserToken.confirmation_token(user) + {encoded_token, user_token} = UserToken.build_user_email_token(user, "confirm") Repo.insert!(user_token) UserNotifier.deliver_confirmation_instructions(user, confirmation_url_fun.(encoded_token)) :ok @@ -132,18 +143,18 @@ defmodule Demo.Accounts do and the token is deleted. """ def confirm_user(token) do - with {:ok, query} <- UserToken.confirmation_query(token), - {token, user} <- Repo.one(query), - {:ok, _} <- Repo.transaction(confirmation_multi(user, token)) do + with {:ok, query} <- UserToken.verify_user_email_token_query(token, "confirm"), + %User{} = user <- Repo.one(query), + {:ok, _} <- Repo.transaction(confirmation_multi(user)) do :ok else _ -> :error end end - defp confirmation_multi(user, token) do + defp confirmation_multi(user) do Ecto.Multi.new() |> Ecto.Multi.update(:user, User.confirm_changeset(user)) - |> Ecto.Multi.delete(:user_token, token) + |> Ecto.Multi.delete_all(:user_tokens, UserToken.delete_all_tokens_query(user, "confirm")) end end diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 2269199..315a9c3 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -7,7 +7,6 @@ defmodule Demo.Accounts.User do field :password, :string, virtual: true field :encrypted_password, :string field :confirmed_at, :naive_datetime - has_many :tokens, Demo.Accounts.UserToken, foreign_key: :user_id timestamps() end @@ -28,8 +27,8 @@ defmodule Demo.Accounts.User do |> validate_length(:email, max: 160) |> validate_length(:password, min: 12, max: 80) |> unsafe_validate_unique(:email, Demo.Repo) - |> maybe_encrypt_password() |> unique_constraint(:email) + |> maybe_encrypt_password() end defp maybe_encrypt_password(changeset) do @@ -55,7 +54,7 @@ defmodule Demo.Accounts.User do Bcrypt.verify_pass(password, encrypted_password) end - def valid_password?(_) do + def valid_password?(_, _) do Bcrypt.hash_pwd_salt("unused hash to avoid timing attacks") false end diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex index 5e73100..e330fd8 100644 --- a/lib/demo/accounts/user_token.ex +++ b/lib/demo/accounts/user_token.ex @@ -6,7 +6,7 @@ defmodule Demo.Accounts.UserToken do @rand_size 32 schema "user_tokens" do - field :hashed_token, :binary + field :token, :binary field :context, :string field :sent_to, :string belongs_to :user, Demo.Accounts.User @@ -14,24 +14,71 @@ defmodule Demo.Accounts.UserToken do timestamps(updated_at: false) end - def confirmation_token(user) do - emailable_token(user, "confirm", user.email) + @doc """ + Generates a token that will be stored in a signed place, + such as session or cookie. As they are signed, those + tokens do not need to be hashed. + """ + def build_to_be_signed_token(user, context) do + token = :crypto.strong_rand_bytes(@rand_size) + {token, %Demo.Accounts.UserToken{token: token, context: context, user_id: user.id}} + end + + @doc """ + Checks if the token is valid and returns its underlying lookup query. + + The query returns the user found by the token. + """ + def verify_to_be_signed_token_query(token, context) do + query = + from token in token_and_context_query(token, context), + join: user in assoc(token, :user), + where: token.inserted_at > ago(1, "month"), + select: user + + {:ok, query} + end + + @doc """ + Builds a token with a hashed counter part. + + The non-hashed token is sent to the user e-mail while the + hashed part is stored in the database, to avoid reconstruction. + The token is valid for a week as long as users don't change + their email. + """ + def build_user_email_token(user, context) do + build_hashed_token(user, context, user.email) end - def confirmation_query(token) do + defp build_hashed_token(user, context, sent_to) do + token = :crypto.strong_rand_bytes(@rand_size) + hashed_token = :crypto.hash(@hash_algorithm, token) + + {Base.url_encode64(token, padding: false), + %Demo.Accounts.UserToken{ + token: hashed_token, + context: context, + sent_to: sent_to, + user_id: user.id + }} + end + + @doc """ + Checks if the token is valid and returns its underlying lookup query. + + The query returns the user found by the token. + """ + def verify_user_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) query = - from token in Demo.Accounts.UserToken, + from token in token_and_context_query(hashed_token, context), join: user in assoc(token, :user), - where: - token.hashed_token == ^hashed_token and - token.context == "confirm" and - token.inserted_at > ago(1, "week") and - token.sent_to == user.email, - select: {token, user} + where: token.inserted_at > ago(1, "week") and token.sent_to == user.email, + select: user {:ok, query} @@ -40,16 +87,17 @@ defmodule Demo.Accounts.UserToken do end end - defp emailable_token(user, context, sent_to) do - token = :crypto.strong_rand_bytes(@rand_size) - hashed_token = :crypto.hash(@hash_algorithm, token) + @doc """ + Returns the given token with the given context. + """ + def token_and_context_query(token, context) do + from Demo.Accounts.UserToken, where: [token: ^token, context: ^context] + end - {Base.url_encode64(token, padding: false), - %Demo.Accounts.UserToken{ - hashed_token: hashed_token, - context: context, - sent_to: sent_to, - user_id: user.id - }} + @doc """ + A query that deletes all tokens for user in the given context. + """ + def delete_all_tokens_query(user, context) do + from Demo.Accounts.UserToken, where: [user_id: ^user.id, context: ^context] end end diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index d52331c..ccec677 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -5,27 +5,45 @@ defmodule DemoWeb.UserAuth do alias Demo.Accounts @doc """ - Must be invoked every time after login. + Logs the user in. It deletes the CSRF token and renews the session to avoid fixation attacks. """ def login_user(conn, user) do Plug.CSRFProtection.delete_csrf_token() + token = Accounts.generate_to_be_signed_token(user, "session") conn - |> put_session(:user_id, user.id) + |> put_session(:user_token, token) |> configure_session(renew: true) |> redirect(to: signed_in_path(conn)) end + @doc """ + Logs the user out. + + It clears all session data for safety. If you want to keep + some data in the session, we recommend you to manually copy + the data you want to maintain. + """ + def logout_user(conn) do + user_token = get_session(conn, :user_token) + user_token && Accounts.delete_signed_token(user_token, "session") + + conn + |> clear_session() + |> configure_session(renew: true) + |> redirect(to: "/") + end + @doc """ Authenticates the user by looking into the session and remember me token. """ def authenticate_user(conn, _opts) do - user_id = get_session(conn, :user_id) - user = user_id && Accounts.get_user(user_id) + user_token = get_session(conn, :user_token) + user = user_token && Accounts.get_user_by_signed_token(user_token, "session") assign(conn, :current_user, user) end diff --git a/lib/demo_web/controllers/user_confirmation.ex b/lib/demo_web/controllers/user_confirmation.ex index dcff5b8..2e1d621 100644 --- a/lib/demo_web/controllers/user_confirmation.ex +++ b/lib/demo_web/controllers/user_confirmation.ex @@ -37,7 +37,7 @@ defmodule DemoWeb.UserConfirmationController do :error -> conn |> put_flash(:error, "Confirmation token is invalid or it has expired.") - |> redirect(to: Routes.user_confirmation_path(conn, :new)) + |> redirect(to: "/") end end end diff --git a/lib/demo_web/controllers/user_session_controller.ex b/lib/demo_web/controllers/user_session_controller.ex index 28b243b..b0e8ea8 100644 --- a/lib/demo_web/controllers/user_session_controller.ex +++ b/lib/demo_web/controllers/user_session_controller.ex @@ -22,8 +22,6 @@ defmodule DemoWeb.UserSessionController do def delete(conn, _params) do conn |> put_flash(:info, "Logged out successfully.") - |> clear_session() - |> configure_session(renew: true) - |> redirect(to: "/") + |> UserAuth.logout_user() end end diff --git a/lib/demo_web/templates/user_session/new.html.eex b/lib/demo_web/templates/user_session/new.html.eex index fa8ae92..7373aae 100644 --- a/lib/demo_web/templates/user_session/new.html.eex +++ b/lib/demo_web/templates/user_session/new.html.eex @@ -1,6 +1,6 @@

Login

-<%= form_for :user, Routes.user_session_path(@conn, :create), fn f -> %> +<%= form_for @conn, Routes.user_session_path(@conn, :create), [as: :user], fn f -> %> <%= if @error_message do %>

<%= @error_message %>

diff --git a/priv/repo/migrations/20200316103722_create_auth_tables.exs b/priv/repo/migrations/20200316103722_create_auth_tables.exs index 8529031..76e2b87 100644 --- a/priv/repo/migrations/20200316103722_create_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_auth_tables.exs @@ -13,10 +13,12 @@ defmodule Demo.Repo.Migrations.CreateAuthTables do create table(:user_tokens) do add :user_id, references(:users, on_delete: :delete_all), null: false - add :hashed_token, :binary, null: false + add :token, :binary, null: false add :context, :string, null: false add :sent_to, :string add :inserted_at, :naive_datetime end + + create index(:user_tokens, [:user_id, :token]) end end From 64ed8f9581e83890bad4f9d8676e48507c99eb47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 23 Mar 2020 11:28:57 +0100 Subject: [PATCH 05/76] Password resets --- lib/demo/accounts.ex | 97 ++++++++++++++++++- lib/demo/accounts/user.ex | 18 +++- lib/demo/accounts/user_notifier.ex | 24 +++++ lib/demo/accounts/user_token.ex | 4 +- .../user_registration_controller.ex | 2 +- .../user_reset_password_controller.ex | 59 +++++++++++ lib/demo_web/router.ex | 6 +- .../templates/user_confirmation/new.html.eex | 5 + .../templates/user_registration/new.html.eex | 5 + .../user_reset_password/edit.html.eex | 26 +++++ .../user_reset_password/new.html.eex | 15 +++ .../templates/user_session/new.html.eex | 5 + .../views/user_reset_password_view.ex | 3 + 13 files changed, 259 insertions(+), 10 deletions(-) create mode 100644 lib/demo_web/controllers/user_reset_password_controller.ex create mode 100644 lib/demo_web/templates/user_reset_password/edit.html.eex create mode 100644 lib/demo_web/templates/user_reset_password/new.html.eex create mode 100644 lib/demo_web/views/user_reset_password_view.ex diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 96b47e1..c478e1c 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -6,6 +6,8 @@ defmodule Demo.Accounts do alias Demo.Repo alias Demo.Accounts.{User, UserToken, UserNotifier} + ## Database getters + @doc """ Gets a user by email. @@ -56,6 +58,8 @@ defmodule Demo.Accounts do """ def get_user!(id), do: Repo.get!(User, id) + ## User registration + @doc """ Registers a user. @@ -79,14 +83,16 @@ defmodule Demo.Accounts do ## Examples - iex> change_user(user) + iex> change_user_registration(user) %Ecto.Changeset{data: %User{}} """ - def registration_user(%User{} = user, attrs \\ %{}) do + def change_user_registration(%User{} = user, attrs \\ %{}) do User.registration_changeset(user, attrs) end + ## Session/Remember me + @doc """ Generates a session/cookie token. """ @@ -112,6 +118,8 @@ defmodule Demo.Accounts do :ok end + ## Confirmation + @doc """ Delivers the confirmation e-mail instructions to the given user. @@ -145,16 +153,95 @@ defmodule Demo.Accounts do def confirm_user(token) do with {:ok, query} <- UserToken.verify_user_email_token_query(token, "confirm"), %User{} = user <- Repo.one(query), - {:ok, _} <- Repo.transaction(confirmation_multi(user)) do + {:ok, _} <- Repo.transaction(confirm_user_multi(user)) do :ok else _ -> :error end end - defp confirmation_multi(user) do + defp confirm_user_multi(user) do Ecto.Multi.new() |> Ecto.Multi.update(:user, User.confirm_changeset(user)) - |> Ecto.Multi.delete_all(:user_tokens, UserToken.delete_all_tokens_query(user, "confirm")) + |> Ecto.Multi.delete_all(:confirm, UserToken.delete_all_tokens_query(user, ["confirm"])) + end + + ## Reset passwword + + @doc """ + Delivers the reset password e-mail to the given user. + + ## Examples + + iex> deliver_reset_password_instructions(user, &Routes.user_reset_password_url(conn, :edit)) + :ok + + """ + def deliver_reset_password_instructions(%User{} = user, reset_password_url_fun) + when is_function(reset_password_url_fun, 1) do + {encoded_token, user_token} = UserToken.build_user_email_token(user, "reset_password") + Repo.insert!(user_token) + UserNotifier.deliver_reset_password_instructions(user, reset_password_url_fun.(encoded_token)) + :ok + end + + @doc """ + Gets the user by reset password token. + + ## Examples + + iex> get_user_by_reset_password_token("validtoken-sadsadsa") + %User{} + + iex> get_user_by_reset_password_token("invalidtoken-sadsadsa") + nil + + """ + def get_user_by_reset_password_token(token) do + with {:ok, query} <- UserToken.verify_user_email_token_query(token, "reset_password"), + %User{} = user <- Repo.one(query) do + user + else + _ -> nil + end + end + + @doc """ + Returns an `%Ecto.Changeset{}` for tracking user reset password. + + ## Examples + + iex> change_user_reset_password(user) + %Ecto.Changeset{data: %User{}} + + """ + def change_user_reset_password(user) do + User.reset_password_changeset(user, %{}) + end + + @to_delete_on_reset ~w(reset_password session remember_me) + + @doc """ + Returns an `%Ecto.Changeset{}` for tracking user reset password. + + ## Examples + + + iex> reset_password_user(user, %{password: "new long password", password_confirmation: "new long password"}) + {:ok, %User{}} + + iex> reset_password_user(user, %{password: "valid", password_confirmation: "not the same"}) + {:error, %Ecto.Changeset{}} + + """ + def reset_password_user(user, attrs \\ %{}) do + Ecto.Multi.new() + |> Ecto.Multi.update(:user, User.reset_password_changeset(user, attrs)) + |> Ecto.Multi.delete_all(:tokens, UserToken.delete_all_tokens_query(user, @to_delete_on_reset)) + |> Repo.transaction() + |> case do + {:ok, %{user: user}} -> {:ok, user} + {:error, :user, changeset, _} -> {:error, changeset} + end end end diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 315a9c3..fecf620 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -5,6 +5,7 @@ defmodule Demo.Accounts.User do schema "users" do field :email, :string field :password, :string, virtual: true + field :password_confirmation, :string, virtual: true field :encrypted_password, :string field :confirmed_at, :naive_datetime @@ -25,7 +26,7 @@ defmodule Demo.Accounts.User do |> validate_required([:email, :password]) |> validate_format(:email, "@") |> validate_length(:email, max: 160) - |> validate_length(:password, min: 12, max: 80) + |> validate_password() |> unsafe_validate_unique(:email, Demo.Repo) |> unique_constraint(:email) |> maybe_encrypt_password() @@ -41,6 +42,10 @@ defmodule Demo.Accounts.User do end end + defp validate_password(changeset) do + validate_length(changeset, :password, min: 12, max: 80) + end + @doc """ Verifies the password. @@ -66,4 +71,15 @@ defmodule Demo.Accounts.User do now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second) change(user, confirmed_at: now) end + + @doc """ + Reset password changeset. + """ + def reset_password_changeset(user, attrs) do + user + |> cast(attrs, [:password, :password_confirmation]) + |> validate_required([:password, :password_confirmation]) + |> validate_password() + |> validate_confirmation(:password) + end end diff --git a/lib/demo/accounts/user_notifier.ex b/lib/demo/accounts/user_notifier.ex index 1dbbf20..4e11ad7 100644 --- a/lib/demo/accounts/user_notifier.ex +++ b/lib/demo/accounts/user_notifier.ex @@ -5,6 +5,10 @@ defmodule Demo.Accounts.UserNotifier do # * Swoosh - https://github.com/swoosh/swoosh # * Bamboo - https://github.com/thoughtbot/bamboo # + + @doc """ + Deliver instructions to confirm account. + """ def deliver_confirmation_instructions(user, url) do IO.puts """ @@ -21,4 +25,24 @@ defmodule Demo.Accounts.UserNotifier do ============================== """ end + + @doc """ + Deliver instructions to reset password account. + """ + def deliver_reset_password_instructions(user, url) do + IO.puts """ + + ============================== + + Hi #{user.email}, + + You can reset your password by visiting the url below: + + #{url} + + If you didn't request this change, please ignore this. + + ============================== + """ + end end diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex index e330fd8..39de824 100644 --- a/lib/demo/accounts/user_token.ex +++ b/lib/demo/accounts/user_token.ex @@ -97,7 +97,7 @@ defmodule Demo.Accounts.UserToken do @doc """ A query that deletes all tokens for user in the given context. """ - def delete_all_tokens_query(user, context) do - from Demo.Accounts.UserToken, where: [user_id: ^user.id, context: ^context] + def delete_all_tokens_query(user, [_ | _] = contexts) do + from t in Demo.Accounts.UserToken, where: t.user_id == ^user.id and t.context in ^contexts end end diff --git a/lib/demo_web/controllers/user_registration_controller.ex b/lib/demo_web/controllers/user_registration_controller.ex index 124bea7..b824a89 100644 --- a/lib/demo_web/controllers/user_registration_controller.ex +++ b/lib/demo_web/controllers/user_registration_controller.ex @@ -6,7 +6,7 @@ defmodule DemoWeb.UserRegistrationController do alias DemoWeb.UserAuth def new(conn, _params) do - changeset = Accounts.registration_user(%User{}) + changeset = Accounts.change_user_registration(%User{}) render(conn, "new.html", changeset: changeset) end diff --git a/lib/demo_web/controllers/user_reset_password_controller.ex b/lib/demo_web/controllers/user_reset_password_controller.ex new file mode 100644 index 0000000..a7b31f1 --- /dev/null +++ b/lib/demo_web/controllers/user_reset_password_controller.ex @@ -0,0 +1,59 @@ +defmodule DemoWeb.UserResetPasswordController do + use DemoWeb, :controller + + alias Demo.Accounts + alias Demo.Accounts.User + + plug :get_user_by_reset_password_token when action in [:edit, :update] + + def new(conn, _params) do + render(conn, "new.html") + end + + def create(conn, %{"user" => %{"email" => email}}) do + if user = Accounts.get_user_by_email(email) do + Accounts.deliver_reset_password_instructions( + user, + &Routes.user_reset_password_url(conn, :edit, &1) + ) + end + + # Regardless of the outcome, show an impartial success/error message. + conn + |> put_flash( + :info, + "If your e-mail is in our system, you receive instructions to reset your password shortly." + ) + |> redirect(to: "/") + end + + def edit(conn, _params) do + render(conn, "edit.html", changeset: Accounts.change_user_reset_password(%User{})) + end + + # Do not login the user after reset password to avoid a + # leaked token giving the user access to the account. + def update(conn, %{"user" => user_params}) do + case Accounts.reset_password_user(conn.assigns.user, user_params) do + {:ok, user} -> + conn + |> put_flash(:info, "Password reset successfully.") + |> redirect(to: Routes.user_session_path(conn, :new)) + + {:error, changeset} -> + render(conn, "edit.html", changeset: changeset) + end + end + + defp get_user_by_reset_password_token(conn, _) do + %{"token" => token} = conn.params + + if user = Accounts.get_user_by_reset_password_token(token) do + conn |> assign(:user, user) |> assign(:token, token) + else + conn + |> put_flash(:error, "Reset password token is invalid or it has expired.") + |> redirect(to: "/") + end + end +end diff --git a/lib/demo_web/router.ex b/lib/demo_web/router.ex index 40333f3..5c74d20 100644 --- a/lib/demo_web/router.ex +++ b/lib/demo_web/router.ex @@ -37,6 +37,10 @@ defmodule DemoWeb.Router do post "/users/register", UserRegistrationController, :create get "/users/login", UserSessionController, :new post "/users/login", UserSessionController, :create + get "/users/reset_password", UserResetPasswordController, :new + post "/users/reset_password", UserResetPasswordController, :create + get "/users/reset_password/:token", UserResetPasswordController, :edit + put "/users/reset_password/:token", UserResetPasswordController, :update end scope "/", DemoWeb do @@ -48,7 +52,7 @@ defmodule DemoWeb.Router do scope "/", DemoWeb do pipe_through [:browser] - get "/users/confirm/new", UserConfirmationController, :new + get "/users/confirm", UserConfirmationController, :new post "/users/confirm", UserConfirmationController, :create get "/users/confirm/:token", UserConfirmationController, :confirm end diff --git a/lib/demo_web/templates/user_confirmation/new.html.eex b/lib/demo_web/templates/user_confirmation/new.html.eex index 2f58f4d..70983e8 100644 --- a/lib/demo_web/templates/user_confirmation/new.html.eex +++ b/lib/demo_web/templates/user_confirmation/new.html.eex @@ -8,3 +8,8 @@ <%= submit "Resend confirmation instructions" %>
<% end %> + +

+ <%= link "Register", to: Routes.user_registration_path(@conn, :new) %> | + <%= link "Login", to: Routes.user_session_path(@conn, :new) %> +

diff --git a/lib/demo_web/templates/user_registration/new.html.eex b/lib/demo_web/templates/user_registration/new.html.eex index ee93159..dfb96a2 100644 --- a/lib/demo_web/templates/user_registration/new.html.eex +++ b/lib/demo_web/templates/user_registration/new.html.eex @@ -19,3 +19,8 @@ <%= submit "Register" %>
<% end %> + +

+ <%= link "Login", to: Routes.user_session_path(@conn, :new) %> | + <%= link "Forgot your password?", to: Routes.user_reset_password_path(@conn, :new) %> +

diff --git a/lib/demo_web/templates/user_reset_password/edit.html.eex b/lib/demo_web/templates/user_reset_password/edit.html.eex new file mode 100644 index 0000000..f54cfbe --- /dev/null +++ b/lib/demo_web/templates/user_reset_password/edit.html.eex @@ -0,0 +1,26 @@ +

Reset password

+ +<%= form_for @changeset, Routes.user_reset_password_path(@conn, :update, @token), fn f -> %> + <%= if @changeset.action do %> +
+

Oops, something went wrong! Please check the errors below.

+
+ <% end %> + + <%= label f, :password, "New password" %> + <%= password_input f, :password, required: true %> + <%= error_tag f, :password %> + + <%= label f, :password_confirmation, "Confirm new password" %> + <%= password_input f, :password_confirmation, required: true %> + <%= error_tag f, :password_confirmation %> + +
+ <%= submit "Reset password" %> +
+<% end %> + +

+ <%= link "Register", to: Routes.user_registration_path(@conn, :new) %> | + <%= link "Login", to: Routes.user_session_path(@conn, :new) %> +

diff --git a/lib/demo_web/templates/user_reset_password/new.html.eex b/lib/demo_web/templates/user_reset_password/new.html.eex new file mode 100644 index 0000000..f0de821 --- /dev/null +++ b/lib/demo_web/templates/user_reset_password/new.html.eex @@ -0,0 +1,15 @@ +

Forgot your password?

+ +<%= form_for :user, Routes.user_reset_password_path(@conn, :create), fn f -> %> + <%= label f, :email %> + <%= text_input f, :email, required: true %> + +
+ <%= submit "Send instructions to reset password" %> +
+<% end %> + +

+ <%= link "Register", to: Routes.user_registration_path(@conn, :new) %> | + <%= link "Login", to: Routes.user_session_path(@conn, :new) %> +

diff --git a/lib/demo_web/templates/user_session/new.html.eex b/lib/demo_web/templates/user_session/new.html.eex index 7373aae..e3b92e3 100644 --- a/lib/demo_web/templates/user_session/new.html.eex +++ b/lib/demo_web/templates/user_session/new.html.eex @@ -17,3 +17,8 @@ <%= submit "Login" %> <% end %> + +

+ <%= link "Register", to: Routes.user_registration_path(@conn, :new) %> | + <%= link "Forgot your password?", to: Routes.user_reset_password_path(@conn, :new) %> +

diff --git a/lib/demo_web/views/user_reset_password_view.ex b/lib/demo_web/views/user_reset_password_view.ex new file mode 100644 index 0000000..bcd50ba --- /dev/null +++ b/lib/demo_web/views/user_reset_password_view.ex @@ -0,0 +1,3 @@ +defmodule DemoWeb.UserResetPasswordView do + use DemoWeb, :view +end From d4ea2b468270c509bfa40b41c03ed49d3ac93539 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 23 Mar 2020 17:27:19 +0100 Subject: [PATCH 06/76] E-mail change --- lib/demo/accounts.ex | 87 ++++++++++++++++++- lib/demo/accounts/user.ex | 77 ++++++++++++---- lib/demo/accounts/user_notifier.ex | 20 +++++ lib/demo/accounts/user_token.ex | 29 ++++++- .../user_reset_password_controller.ex | 7 +- .../controllers/user_settings_controller.ex | 56 ++++++++++++ lib/demo_web/router.ex | 4 + .../templates/layout/_user_menu.html.eex | 1 + .../templates/user_settings/edit.html.eex | 49 +++++++++++ lib/demo_web/views/user_settings_view.ex | 3 + 10 files changed, 304 insertions(+), 29 deletions(-) create mode 100644 lib/demo_web/controllers/user_settings_controller.ex create mode 100644 lib/demo_web/templates/user_settings/edit.html.eex create mode 100644 lib/demo_web/views/user_settings_view.ex diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index c478e1c..594083c 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -91,6 +91,87 @@ defmodule Demo.Accounts do User.registration_changeset(user, attrs) end + ## Settings + + @doc """ + Returns an `%Ecto.Changeset{}` for changing the user e-mail. + + ## Examples + + iex> change_user_email(user) + %Ecto.Changeset{data: %User{}} + + """ + def change_user_email(user, attrs \\ %{}) do + User.email_changeset(user, attrs) + end + + @doc """ + Returns an `%Ecto.Changeset{}` for changing the user password. + + ## Examples + + iex> change_user_password(user) + %Ecto.Changeset{data: %User{}} + + """ + def change_user_password(user, attrs \\ %{}) do + User.password_changeset(user, attrs) + end + + @doc """ + Emulates that the e-mail will change without actually changing + it in the database. + """ + def apply_user_email(user, password, attrs) do + user + |> User.email_changeset(attrs) + |> User.validate_current_password(password) + |> Ecto.Changeset.apply_action(:update) + end + + @doc """ + Updates the user e-mail in token. + + If the token matches, the user email is updated and the token is deleted. + """ + def update_user_email(user, token) do + context = "change:#{user.email}" + + with {:ok, query} <- UserToken.verify_user_change_email_token_query(token, context), + %UserToken{sent_to: email} <- Repo.one(query), + {:ok, _} <- Repo.transaction(user_email_multi(user, email, context)) do + :ok + else + _ -> :error + end + end + + defp user_email_multi(user, email, context) do + Ecto.Multi.new() + |> Ecto.Multi.update(:user, User.email_changeset(user, %{email: email})) + |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, [context])) + end + + @doc """ + Delivers the update e-mail instructions to the given user. + + ## Examples + + iex> deliver_update_email_instructions(user, current_email, &Routes.user_update_email_url(conn, :edit)) + :ok + + """ + def deliver_update_email_instructions(%User{} = user, current_email, update_email_url_fun) + when is_function(update_email_url_fun, 1) do + {encoded_token, user_token} = + UserToken.build_user_email_token(user, "change:#{current_email}") + + Repo.insert!(user_token) + UserNotifier.deliver_update_email_instructions(user, update_email_url_fun.(encoded_token)) + :ok + end + ## Session/Remember me @doc """ @@ -163,7 +244,7 @@ defmodule Demo.Accounts do defp confirm_user_multi(user) do Ecto.Multi.new() |> Ecto.Multi.update(:user, User.confirm_changeset(user)) - |> Ecto.Multi.delete_all(:confirm, UserToken.delete_all_tokens_query(user, ["confirm"])) + |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, ["confirm"])) end ## Reset passwword @@ -219,8 +300,6 @@ defmodule Demo.Accounts do User.reset_password_changeset(user, %{}) end - @to_delete_on_reset ~w(reset_password session remember_me) - @doc """ Returns an `%Ecto.Changeset{}` for tracking user reset password. @@ -237,7 +316,7 @@ defmodule Demo.Accounts do def reset_password_user(user, attrs \\ %{}) do Ecto.Multi.new() |> Ecto.Multi.update(:user, User.reset_password_changeset(user, attrs)) - |> Ecto.Multi.delete_all(:tokens, UserToken.delete_all_tokens_query(user, @to_delete_on_reset)) + |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, :all)) |> Repo.transaction() |> case do {:ok, %{user: user}} -> {:ok, user} diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index fecf620..968ad66 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -5,7 +5,6 @@ defmodule Demo.Accounts.User do schema "users" do field :email, :string field :password, :string, virtual: true - field :password_confirmation, :string, virtual: true field :encrypted_password, :string field :confirmed_at, :naive_datetime @@ -24,11 +23,8 @@ defmodule Demo.Accounts.User do user |> cast(attrs, [:email, :password]) |> validate_required([:email, :password]) - |> validate_format(:email, "@") - |> validate_length(:email, max: 160) + |> validate_email() |> validate_password() - |> unsafe_validate_unique(:email, Demo.Repo) - |> unique_constraint(:email) |> maybe_encrypt_password() end @@ -42,26 +38,40 @@ defmodule Demo.Accounts.User do end end + defp validate_email(changeset) do + changeset + |> validate_format(:email, "@") + |> validate_length(:email, max: 160) + |> unsafe_validate_unique(:email, Demo.Repo) + |> unique_constraint(:email) + end + defp validate_password(changeset) do validate_length(changeset, :password, min: 12, max: 80) end @doc """ - Verifies the password. - - Returns the given user if valid, + A user changeset for changing the e-mail. - If there is no user or the user doesn't have a password, - we encrypt a blank password to avoid timing attacks. + It requires the e-mail to change otherwise an error is added. """ - def valid_password?(%Demo.Accounts.User{encrypted_password: encrypted_password}, password) - when is_binary(encrypted_password) do - Bcrypt.verify_pass(password, encrypted_password) + def email_changeset(user, attrs) do + user + |> cast(attrs, [:email]) + |> validate_email() + |> case do + %{changes: %{email: _}} = changeset -> changeset + %{} = changeset -> add_error(changeset, :email, "did not change") + end end - def valid_password?(_, _) do - Bcrypt.hash_pwd_salt("unused hash to avoid timing attacks") - false + @doc """ + A user changeset for changing the password. + """ + def password_changeset(user, attrs) do + user + |> cast(attrs, [:password]) + |> validate_password() end @doc """ @@ -73,13 +83,42 @@ defmodule Demo.Accounts.User do end @doc """ - Reset password changeset. + A user changeset for resetting password. """ def reset_password_changeset(user, attrs) do user - |> cast(attrs, [:password, :password_confirmation]) - |> validate_required([:password, :password_confirmation]) + |> cast(attrs, [:password]) + |> validate_required([:password]) |> validate_password() |> validate_confirmation(:password) end + + @doc """ + Verifies the password. + + Returns the given user if valid, + + If there is no user or the user doesn't have a password, + we encrypt a blank password to avoid timing attacks. + """ + def valid_password?(%Demo.Accounts.User{encrypted_password: encrypted_password}, password) + when is_binary(encrypted_password) do + Bcrypt.verify_pass(password, encrypted_password) + end + + def valid_password?(_, _) do + Bcrypt.hash_pwd_salt("unused hash to avoid timing attacks") + false + end + + @doc """ + Validates the current password otherwise adds an error to the changeset. + """ + def validate_current_password(changeset, password) do + if valid_password?(changeset.data, password) do + changeset + else + add_error(changeset, :current_password, "is not valid") + end + end end diff --git a/lib/demo/accounts/user_notifier.ex b/lib/demo/accounts/user_notifier.ex index 4e11ad7..9006529 100644 --- a/lib/demo/accounts/user_notifier.ex +++ b/lib/demo/accounts/user_notifier.ex @@ -45,4 +45,24 @@ defmodule Demo.Accounts.UserNotifier do ============================== """ end + + @doc """ + Deliver instructions to update your e-mail. + """ + def deliver_update_email_instructions(user, url) do + IO.puts """ + + ============================== + + Hi #{user.email}, + + You can change your e-mail by visiting the url below: + + #{url} + + If you didn't request this change, please ignore this. + + ============================== + """ + end end diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex index 39de824..a39ffdd 100644 --- a/lib/demo/accounts/user_token.ex +++ b/lib/demo/accounts/user_token.ex @@ -87,6 +87,27 @@ defmodule Demo.Accounts.UserToken do end end + @doc """ + Checks if the token is valid and returns its underlying lookup query. + + The query returns the user found by the token. + """ + def verify_user_change_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) + + query = + from token in token_and_context_query(hashed_token, context), + where: token.inserted_at > ago(1, "week") + + {:ok, query} + + :error -> + :error + end + end + @doc """ Returns the given token with the given context. """ @@ -95,9 +116,13 @@ defmodule Demo.Accounts.UserToken do end @doc """ - A query that deletes all tokens for user in the given context. + Gets all tokens for the given user for the given contexts. """ - def delete_all_tokens_query(user, [_ | _] = contexts) do + def user_and_contexts_query(user, :all) do + from t in Demo.Accounts.UserToken, where: t.user_id == ^user.id + end + + def user_and_contexts_query(user, [_ | _] = contexts) do from t in Demo.Accounts.UserToken, where: t.user_id == ^user.id and t.context in ^contexts end end diff --git a/lib/demo_web/controllers/user_reset_password_controller.ex b/lib/demo_web/controllers/user_reset_password_controller.ex index a7b31f1..3f3fad9 100644 --- a/lib/demo_web/controllers/user_reset_password_controller.ex +++ b/lib/demo_web/controllers/user_reset_password_controller.ex @@ -2,7 +2,6 @@ defmodule DemoWeb.UserResetPasswordController do use DemoWeb, :controller alias Demo.Accounts - alias Demo.Accounts.User plug :get_user_by_reset_password_token when action in [:edit, :update] @@ -28,14 +27,14 @@ defmodule DemoWeb.UserResetPasswordController do end def edit(conn, _params) do - render(conn, "edit.html", changeset: Accounts.change_user_reset_password(%User{})) + render(conn, "edit.html", changeset: Accounts.change_user_reset_password(conn.assigns.user)) end # Do not login the user after reset password to avoid a # leaked token giving the user access to the account. def update(conn, %{"user" => user_params}) do case Accounts.reset_password_user(conn.assigns.user, user_params) do - {:ok, user} -> + {:ok, _} -> conn |> put_flash(:info, "Password reset successfully.") |> redirect(to: Routes.user_session_path(conn, :new)) @@ -45,7 +44,7 @@ defmodule DemoWeb.UserResetPasswordController do end end - defp get_user_by_reset_password_token(conn, _) do + defp get_user_by_reset_password_token(conn, _opts) do %{"token" => token} = conn.params if user = Accounts.get_user_by_reset_password_token(token) do diff --git a/lib/demo_web/controllers/user_settings_controller.ex b/lib/demo_web/controllers/user_settings_controller.ex new file mode 100644 index 0000000..696327f --- /dev/null +++ b/lib/demo_web/controllers/user_settings_controller.ex @@ -0,0 +1,56 @@ +defmodule DemoWeb.UserSettingsController do + use DemoWeb, :controller + + alias Demo.Accounts + + plug :assign_email_and_password_changesets + + def edit(conn, _params) do + render(conn, "edit.html") + end + + def update_email(conn, %{"current_password" => password, "user" => user_params}) do + user = conn.assigns.current_user + + case Accounts.validate_user_email(user, password, user_params) do + {:ok, applied_user} -> + Accounts.deliver_update_email_instructions( + applied_user, + user.email, + &Routes.user_settings_url(conn, :confirm_email, &1) + ) + + conn + |> put_flash( + :info, + "A link to confirm your e-mail change has been sent to the new address." + ) + |> redirect(to: Routes.user_settings_path(conn, :edit)) + + {:error, changeset} -> + render(conn, "edit.html", email_changeset: changeset) + end + end + + def confirm_email(conn, %{"token" => token}) do + case Accounts.update_user_email(conn.assigns.current_user, token) do + :ok -> + conn + |> put_flash(:info, "E-mail changed successfully.") + |> redirect(to: Routes.user_settings_path(conn, :edit)) + + :error -> + conn + |> put_flash(:error, "Email change token is invalid or it has expired.") + |> redirect(to: Routes.user_settings_path(conn, :edit)) + end + end + + defp assign_email_and_password_changesets(conn, _opts) do + user = conn.assigns.current_user + + conn + |> assign(:email_changeset, Accounts.change_user_email(user)) + |> assign(:password_changeset, Accounts.change_user_password(user)) + end +end diff --git a/lib/demo_web/router.ex b/lib/demo_web/router.ex index 5c74d20..ad1ef27 100644 --- a/lib/demo_web/router.ex +++ b/lib/demo_web/router.ex @@ -47,6 +47,10 @@ defmodule DemoWeb.Router do pipe_through [:browser, :require_authenticated_user] delete "/users/logout", UserSessionController, :delete + get "/users/settings", UserSettingsController, :edit + put "/users/settings/update_password", UserSettingsController, :update_password + put "/users/settings/update_email", UserSettingsController, :update_email + get "/users/settings/confirm_email/:token", UserSettingsController, :confirm_email end scope "/", DemoWeb do diff --git a/lib/demo_web/templates/layout/_user_menu.html.eex b/lib/demo_web/templates/layout/_user_menu.html.eex index 4bf844c..76ec857 100644 --- a/lib/demo_web/templates/layout/_user_menu.html.eex +++ b/lib/demo_web/templates/layout/_user_menu.html.eex @@ -1,6 +1,7 @@
    <%= if @current_user do %>
  • <%= @current_user.email %>
    • +
  • <%= link "Settings", to: Routes.user_settings_path(@conn, :edit) %>
  • <%= link "Logout", to: Routes.user_session_path(@conn, :delete), method: :delete %>
    • <% else %>
  • <%= link "Register", to: Routes.user_registration_path(@conn, :new) %>
    • diff --git a/lib/demo_web/templates/user_settings/edit.html.eex b/lib/demo_web/templates/user_settings/edit.html.eex new file mode 100644 index 0000000..1de8084 --- /dev/null +++ b/lib/demo_web/templates/user_settings/edit.html.eex @@ -0,0 +1,49 @@ +

    Settings

    + +

    Change e-mail

    + +<%= form_for @email_changeset, Routes.user_settings_path(@conn, :update_email), fn f -> %> + <%= if @email_changeset.action do %> +
    +

    Oops, something went wrong! Please check the errors below.

    +
    + <% end %> + + <%= label f, :email %> + <%= text_input f, :email, required: true %> + <%= error_tag f, :email %> + + <%= label f, :current_password %> + <%= password_input f, :current_password, required: true, name: "current_password" %> + <%= error_tag f, :current_password %> + +
    + <%= submit "Change e-mail" %> +
    +<% end %> + +

    Change password

    + +<%= form_for @password_changeset, Routes.user_settings_path(@conn, :update_password), fn f -> %> + <%= if @password_changeset.action do %> +
    +

    Oops, something went wrong! Please check the errors below.

    +
    + <% end %> + + <%= label f, :password, "New password" %> + <%= password_input f, :password, required: true %> + <%= error_tag f, :password %> + + <%= label f, :password_confirmation, "Confirm new password" %> + <%= password_input f, :password_confirmation, required: true %> + <%= error_tag f, :password_confirmation %> + + <%= label f, :current_password %> + <%= password_input f, :current_password, required: true, name: "current_password" %> + <%= error_tag f, :current_password %> + +
    + <%= submit "Change password" %> +
    +<% end %> diff --git a/lib/demo_web/views/user_settings_view.ex b/lib/demo_web/views/user_settings_view.ex new file mode 100644 index 0000000..845f9fe --- /dev/null +++ b/lib/demo_web/views/user_settings_view.ex @@ -0,0 +1,3 @@ +defmodule DemoWeb.UserSettingsView do + use DemoWeb, :view +end From b677abcd835060563091934ad9ace57e6301972b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 23 Mar 2020 17:50:47 +0100 Subject: [PATCH 07/76] Update password --- lib/demo/accounts.ex | 69 +++++++++++-------- lib/demo/accounts/user.ex | 40 +++++------ .../user_reset_password_controller.ex | 4 +- .../controllers/user_settings_controller.ex | 16 ++++- 4 files changed, 75 insertions(+), 54 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 594083c..24a746e 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -107,21 +107,17 @@ defmodule Demo.Accounts do end @doc """ - Returns an `%Ecto.Changeset{}` for changing the user password. + Emulates that the e-mail will change without actually changing + it in the database. ## Examples - iex> change_user_password(user) - %Ecto.Changeset{data: %User{}} + iex> apply_user_email(user, "valid password", %{email: ...}) + {:ok, %User{}} - """ - def change_user_password(user, attrs \\ %{}) do - User.password_changeset(user, attrs) - end + iex> apply_user_email(user, "invalid password", %{email: ...}) + {:error, %Ecto.Changeset{}} - @doc """ - Emulates that the e-mail will change without actually changing - it in the database. """ def apply_user_email(user, password, attrs) do user @@ -172,6 +168,38 @@ defmodule Demo.Accounts do :ok end + @doc """ + Returns an `%Ecto.Changeset{}` for changing the user password. + + ## Examples + + iex> change_user_password(user) + %Ecto.Changeset{data: %User{}} + + """ + def change_user_password(user, attrs \\ %{}) do + User.password_changeset(user, attrs) + end + + @doc """ + Returns an `%Ecto.Changeset{}` for changing the user password. + + ## Examples + + iex> update_user_password(user, "valid password", %{password: ...}) + {:ok, %User{}} + + iex> update_user_password(user, "invalid password", %{password: ...}) + {:error, %Ecto.Changeset{}} + + """ + def update_user_password(user, password, attrs \\ %{}) do + user + |> User.password_changeset(attrs) + |> User.validate_current_password(password) + |> Repo.update() + end + ## Session/Remember me @doc """ @@ -290,32 +318,19 @@ defmodule Demo.Accounts do @doc """ Returns an `%Ecto.Changeset{}` for tracking user reset password. - ## Examples - - iex> change_user_reset_password(user) - %Ecto.Changeset{data: %User{}} - - """ - def change_user_reset_password(user) do - User.reset_password_changeset(user, %{}) - end - - @doc """ - Returns an `%Ecto.Changeset{}` for tracking user reset password. - ## Examples - iex> reset_password_user(user, %{password: "new long password", password_confirmation: "new long password"}) + iex> reset_user_password(user, %{password: "new long password", password_confirmation: "new long password"}) {:ok, %User{}} - iex> reset_password_user(user, %{password: "valid", password_confirmation: "not the same"}) + iex> reset_user_password(user, %{password: "valid", password_confirmation: "not the same"}) {:error, %Ecto.Changeset{}} """ - def reset_password_user(user, attrs \\ %{}) do + def reset_user_password(user, attrs \\ %{}) do Ecto.Multi.new() - |> Ecto.Multi.update(:user, User.reset_password_changeset(user, attrs)) + |> Ecto.Multi.update(:user, User.password_changeset(user, attrs)) |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, :all)) |> Repo.transaction() |> case do diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 968ad66..8e6c7a3 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -22,24 +22,13 @@ defmodule Demo.Accounts.User do def registration_changeset(user, attrs) do user |> cast(attrs, [:email, :password]) - |> validate_required([:email, :password]) |> validate_email() |> validate_password() - |> maybe_encrypt_password() - end - - defp maybe_encrypt_password(changeset) do - password = get_change(changeset, :password) - - if password && changeset.valid? do - put_change(changeset, :encrypted_password, Bcrypt.hash_pwd_salt(password)) - else - changeset - end end defp validate_email(changeset) do changeset + |> validate_required([:email]) |> validate_format(:email, "@") |> validate_length(:email, max: 160) |> unsafe_validate_unique(:email, Demo.Repo) @@ -47,7 +36,20 @@ defmodule Demo.Accounts.User do end defp validate_password(changeset) do - validate_length(changeset, :password, min: 12, max: 80) + changeset + |> validate_required([:password]) + |> validate_length(:password, min: 12, max: 80) + |> maybe_encrypt_password() + end + + defp maybe_encrypt_password(changeset) do + password = get_change(changeset, :password) + + if password && changeset.valid? do + put_change(changeset, :encrypted_password, Bcrypt.hash_pwd_salt(password)) + else + changeset + end end @doc """ @@ -71,6 +73,7 @@ defmodule Demo.Accounts.User do def password_changeset(user, attrs) do user |> cast(attrs, [:password]) + |> validate_confirmation(:password) |> validate_password() end @@ -82,17 +85,6 @@ defmodule Demo.Accounts.User do change(user, confirmed_at: now) end - @doc """ - A user changeset for resetting password. - """ - def reset_password_changeset(user, attrs) do - user - |> cast(attrs, [:password]) - |> validate_required([:password]) - |> validate_password() - |> validate_confirmation(:password) - end - @doc """ Verifies the password. diff --git a/lib/demo_web/controllers/user_reset_password_controller.ex b/lib/demo_web/controllers/user_reset_password_controller.ex index 3f3fad9..06d1e52 100644 --- a/lib/demo_web/controllers/user_reset_password_controller.ex +++ b/lib/demo_web/controllers/user_reset_password_controller.ex @@ -27,13 +27,13 @@ defmodule DemoWeb.UserResetPasswordController do end def edit(conn, _params) do - render(conn, "edit.html", changeset: Accounts.change_user_reset_password(conn.assigns.user)) + render(conn, "edit.html", changeset: Accounts.change_user_password(conn.assigns.user)) end # Do not login the user after reset password to avoid a # leaked token giving the user access to the account. def update(conn, %{"user" => user_params}) do - case Accounts.reset_password_user(conn.assigns.user, user_params) do + case Accounts.reset_user_password(conn.assigns.user, user_params) do {:ok, _} -> conn |> put_flash(:info, "Password reset successfully.") diff --git a/lib/demo_web/controllers/user_settings_controller.ex b/lib/demo_web/controllers/user_settings_controller.ex index 696327f..799647c 100644 --- a/lib/demo_web/controllers/user_settings_controller.ex +++ b/lib/demo_web/controllers/user_settings_controller.ex @@ -12,7 +12,7 @@ defmodule DemoWeb.UserSettingsController do def update_email(conn, %{"current_password" => password, "user" => user_params}) do user = conn.assigns.current_user - case Accounts.validate_user_email(user, password, user_params) do + case Accounts.apply_user_email(user, password, user_params) do {:ok, applied_user} -> Accounts.deliver_update_email_instructions( applied_user, @@ -46,6 +46,20 @@ defmodule DemoWeb.UserSettingsController do end end + def update_password(conn, %{"current_password" => password, "user" => user_params}) do + user = conn.assigns.current_user + + case Accounts.update_user_password(user, password, user_params) do + {:ok, _} -> + conn + |> put_flash(:info, "Password updated successfully.") + |> redirect(to: Routes.user_settings_path(conn, :edit)) + + {:error, changeset} -> + render(conn, "edit.html", password_changeset: changeset) + end + end + defp assign_email_and_password_changesets(conn, _opts) do user = conn.assigns.current_user From 1befca8c04ef884b16ab6dc8e1e1930df3540ade Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 23 Mar 2020 17:55:51 +0100 Subject: [PATCH 08/76] Redirect to where the user was --- lib/demo/accounts/user_notifier.ex | 12 ++++++------ lib/demo_web/controllers/user_auth.ex | 10 ++++++++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/demo/accounts/user_notifier.ex b/lib/demo/accounts/user_notifier.ex index 9006529..2a20f06 100644 --- a/lib/demo/accounts/user_notifier.ex +++ b/lib/demo/accounts/user_notifier.ex @@ -10,7 +10,7 @@ defmodule Demo.Accounts.UserNotifier do Deliver instructions to confirm account. """ def deliver_confirmation_instructions(user, url) do - IO.puts """ + IO.puts(""" ============================== @@ -23,14 +23,14 @@ defmodule Demo.Accounts.UserNotifier do If you didn't create an account with us, please ignore this. ============================== - """ + """) end @doc """ Deliver instructions to reset password account. """ def deliver_reset_password_instructions(user, url) do - IO.puts """ + IO.puts(""" ============================== @@ -43,14 +43,14 @@ defmodule Demo.Accounts.UserNotifier do If you didn't request this change, please ignore this. ============================== - """ + """) end @doc """ Deliver instructions to update your e-mail. """ def deliver_update_email_instructions(user, url) do - IO.puts """ + IO.puts(""" ============================== @@ -63,6 +63,6 @@ defmodule Demo.Accounts.UserNotifier do If you didn't request this change, please ignore this. ============================== - """ + """) end end diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index ccec677..0552cfe 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -3,6 +3,7 @@ defmodule DemoWeb.UserAuth do import Phoenix.Controller alias Demo.Accounts + alias DemoWeb.Router.Helpers, as: Routes @doc """ Logs the user in. @@ -14,10 +15,13 @@ defmodule DemoWeb.UserAuth do Plug.CSRFProtection.delete_csrf_token() token = Accounts.generate_to_be_signed_token(user, "session") + user_return_to = get_session(conn, :user_return_to) + delete_session(conn, :user_return_to) + conn |> put_session(:user_token, token) |> configure_session(renew: true) - |> redirect(to: signed_in_path(conn)) + |> redirect(to: user_return_to || signed_in_path(conn)) end @doc """ @@ -71,7 +75,9 @@ defmodule DemoWeb.UserAuth do conn else conn - |> redirect(to: signed_in_path(conn)) + |> put_flash(:error, "You must be authenticated to access this page.") + |> put_session(:user_return_to, conn.request_path) + |> redirect(to: Routes.user_session_path(conn, :new)) |> halt() end end From 66250190fb59933c16c9017833374cbd545ea227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 24 Mar 2020 11:40:46 +0100 Subject: [PATCH 09/76] Add remember me cookie --- lib/demo/accounts.ex | 16 ++++---- lib/demo/accounts/user_token.ex | 10 ++--- lib/demo_web/controllers/user_auth.ex | 40 ++++++++++++++++--- .../controllers/user_session_controller.ex | 6 ++- .../templates/user_session/new.html.eex | 3 ++ mix.lock | 2 +- 6 files changed, 56 insertions(+), 21 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 24a746e..9054c31 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -200,13 +200,13 @@ defmodule Demo.Accounts do |> Repo.update() end - ## Session/Remember me + ## Session @doc """ - Generates a session/cookie token. + Generates a session token. """ - def generate_to_be_signed_token(user, context) do - {token, user_token} = UserToken.build_to_be_signed_token(user, context) + def generate_session_token(user) do + {token, user_token} = UserToken.build_session_token(user) Repo.insert!(user_token) token end @@ -214,16 +214,16 @@ defmodule Demo.Accounts do @doc """ Gets the user with the given signed token. """ - def get_user_by_signed_token(token, context) do - {:ok, query} = UserToken.verify_to_be_signed_token_query(token, context) + def get_user_by_session_token(token) do + {:ok, query} = UserToken.verify_session_token_query(token) Repo.one(query) end @doc """ Deletes the signed token with the given context. """ - def delete_signed_token(token, context) do - Repo.delete_all(UserToken.token_and_context_query(token, context)) + def delete_session_token(token) do + Repo.delete_all(UserToken.token_and_context_query(token, "session")) :ok end diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex index a39ffdd..384431e 100644 --- a/lib/demo/accounts/user_token.ex +++ b/lib/demo/accounts/user_token.ex @@ -19,9 +19,9 @@ defmodule Demo.Accounts.UserToken do such as session or cookie. As they are signed, those tokens do not need to be hashed. """ - def build_to_be_signed_token(user, context) do + def build_session_token(user) do token = :crypto.strong_rand_bytes(@rand_size) - {token, %Demo.Accounts.UserToken{token: token, context: context, user_id: user.id}} + {token, %Demo.Accounts.UserToken{token: token, context: "session", user_id: user.id}} end @doc """ @@ -29,11 +29,11 @@ defmodule Demo.Accounts.UserToken do The query returns the user found by the token. """ - def verify_to_be_signed_token_query(token, context) do + def verify_session_token_query(token) do query = - from token in token_and_context_query(token, context), + from token in token_and_context_query(token, "session"), join: user in assoc(token, :user), - where: token.inserted_at > ago(1, "month"), + where: token.inserted_at > ago(60, "day"), select: user {:ok, query} diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index 0552cfe..c35d127 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -5,15 +5,21 @@ defmodule DemoWeb.UserAuth do alias Demo.Accounts alias DemoWeb.Router.Helpers, as: Routes + # Make the remember me cookie valid for 60 days. + # If you want bump or reduce this value, also change + # the token expiry itself in UserToken. + @max_age 60 * 60 * 24 * 60 + @remember_me_cookie "user_remember_me" + @doc """ Logs the user in. It deletes the CSRF token and renews the session to avoid fixation attacks. """ - def login_user(conn, user) do + def login_user(conn, user, params \\ %{}) do Plug.CSRFProtection.delete_csrf_token() - token = Accounts.generate_to_be_signed_token(user, "session") + token = Accounts.generate_session_token(user) user_return_to = get_session(conn, :user_return_to) delete_session(conn, :user_return_to) @@ -21,9 +27,18 @@ defmodule DemoWeb.UserAuth do conn |> put_session(:user_token, token) |> configure_session(renew: true) + |> maybe_write_remember_me_cookie(token, params) |> redirect(to: user_return_to || signed_in_path(conn)) end + defp maybe_write_remember_me_cookie(conn, token, %{"remember_me" => "true"}) do + put_resp_cookie(conn, @remember_me_cookie, token, sign: true, max_age: @max_age) + end + + defp maybe_write_remember_me_cookie(conn, _token, _params) do + conn + end + @doc """ Logs the user out. @@ -33,11 +48,12 @@ defmodule DemoWeb.UserAuth do """ def logout_user(conn) do user_token = get_session(conn, :user_token) - user_token && Accounts.delete_signed_token(user_token, "session") + user_token && Accounts.delete_session_token(user_token) conn |> clear_session() |> configure_session(renew: true) + |> delete_resp_cookie(@remember_me_cookie) |> redirect(to: "/") end @@ -46,11 +62,25 @@ defmodule DemoWeb.UserAuth do and remember me token. """ def authenticate_user(conn, _opts) do - user_token = get_session(conn, :user_token) - user = user_token && Accounts.get_user_by_signed_token(user_token, "session") + {user_token, conn} = ensure_user_token(conn) + user = user_token && Accounts.get_user_by_session_token(user_token) assign(conn, :current_user, user) end + defp ensure_user_token(conn) do + if user_token = get_session(conn, :user_token) do + {user_token, conn} + else + conn = fetch_cookies(conn, signed: [@remember_me_cookie]) + + if user_token = conn.req_cookies[@remember_me_cookie] do + {user_token, put_session(conn, :user_token, user_token)} + else + {nil, conn} + end + end + end + @doc """ Used for routes that requires the user to not be authenticated. """ diff --git a/lib/demo_web/controllers/user_session_controller.ex b/lib/demo_web/controllers/user_session_controller.ex index b0e8ea8..646749c 100644 --- a/lib/demo_web/controllers/user_session_controller.ex +++ b/lib/demo_web/controllers/user_session_controller.ex @@ -8,9 +8,11 @@ defmodule DemoWeb.UserSessionController do render(conn, "new.html", error_message: nil) end - def create(conn, %{"user" => %{"email" => email, "password" => password}}) do + def create(conn, %{"user" => user_params}) do + %{"email" => email, "password" => password} = user_params + if user = Accounts.get_user_by_email_and_password(email, password) do - UserAuth.login_user(conn, user) + UserAuth.login_user(conn, user, user_params) else render(conn, "new.html", error_message: "Invalid e-mail or password") end diff --git a/lib/demo_web/templates/user_session/new.html.eex b/lib/demo_web/templates/user_session/new.html.eex index e3b92e3..25acc7b 100644 --- a/lib/demo_web/templates/user_session/new.html.eex +++ b/lib/demo_web/templates/user_session/new.html.eex @@ -13,6 +13,9 @@ <%= label f, :password %> <%= password_input f, :password, required: true %> + <%= label f, :remember_me, "Keep me logged in for 60 days" %> + <%= checkbox f, :remember_me %> +
    <%= submit "Login" %>
    diff --git a/mix.lock b/mix.lock index f6e70cf..4659bf8 100644 --- a/mix.lock +++ b/mix.lock @@ -18,7 +18,7 @@ "phoenix_html": {:hex, :phoenix_html, "2.14.0", "d8c6bc28acc8e65f8ea0080ee05aa13d912c8758699283b8d3427b655aabe284", [:mix], [{:plug, "~> 1.5", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "b0bb30eda478a06dbfbe96728061a93833db3861a49ccb516f839ecb08493fbb"}, "phoenix_live_reload": {:hex, :phoenix_live_reload, "1.2.1", "274a4b07c4adbdd7785d45a8b0bb57634d0b4f45b18d2c508b26c0344bd59b8f", [:mix], [{:file_system, "~> 0.2.1 or ~> 0.3", [hex: :file_system, repo: "hexpm", optional: false]}, {:phoenix, "~> 1.4", [hex: :phoenix, repo: "hexpm", optional: false]}], "hexpm", "41b4103a2fa282cfd747d377233baf213c648fdcc7928f432937676532490eee"}, "phoenix_pubsub": {:git, "https://github.com/phoenixframework/phoenix_pubsub.git", "325abd48e0ec164548b84a8bdf2ff357a2ec20a2", []}, - "plug": {:hex, :plug, "1.9.0", "8d7c4e26962283ff9f8f3347bd73838e2413fbc38b7bb5467d5924f68f3a5a4a", [:mix], [{:mime, "~> 1.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.0", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: true]}], "hexpm", "9902eda2c52ada2a096434682e99a2493f5d06a94d6ac6bcfff9805f952350f1"}, + "plug": {:hex, :plug, "1.10.0", "6508295cbeb4c654860845fb95260737e4a8838d34d115ad76cd487584e2fc4d", [:mix], [{:mime, "~> 1.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.1.1 or ~> 1.2", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: true]}], "hexpm", "422a9727e667be1bf5ab1de03be6fa0ad67b775b2d84ed908f3264415ef29d4a"}, "plug_cowboy": {:hex, :plug_cowboy, "2.1.2", "8b0addb5908c5238fac38e442e81b6fcd32788eaa03246b4d55d147c47c5805e", [:mix], [{:cowboy, "~> 2.5", [hex: :cowboy, repo: "hexpm", optional: false]}, {:plug, "~> 1.7", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "7d722581ce865a237e14da6d946f92704101740a256bd13ec91e63c0b122fc70"}, "plug_crypto": {:hex, :plug_crypto, "1.1.2", "bdd187572cc26dbd95b87136290425f2b580a116d3fb1f564216918c9730d227", [:mix], [], "hexpm", "6b8b608f895b6ffcfad49c37c7883e8df98ae19c6a28113b02aa1e9c5b22d6b5"}, "postgrex": {:hex, :postgrex, "0.15.3", "5806baa8a19a68c4d07c7a624ccdb9b57e89cbc573f1b98099e3741214746ae4", [:mix], [{:connection, "~> 1.0", [hex: :connection, repo: "hexpm", optional: false]}, {:db_connection, "~> 2.1", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.5", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}], "hexpm", "4737ce62a31747b4c63c12b20c62307e51bb4fcd730ca0c32c280991e0606c90"}, From 153b7ce0171213065454d7ba964d04345b4602c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 24 Mar 2020 11:51:08 +0100 Subject: [PATCH 10/76] Remove outdated comments --- lib/demo_web/controllers/user_session_controller.ex | 3 --- lib/demo_web/router.ex | 1 - 2 files changed, 4 deletions(-) diff --git a/lib/demo_web/controllers/user_session_controller.ex b/lib/demo_web/controllers/user_session_controller.ex index 646749c..4bc97b6 100644 --- a/lib/demo_web/controllers/user_session_controller.ex +++ b/lib/demo_web/controllers/user_session_controller.ex @@ -18,9 +18,6 @@ defmodule DemoWeb.UserSessionController do end end - # On logout we clear all session data for safety. - # If you want to keep some data in the session, you can - # explicitly delete the user_id, but do so carefully. def delete(conn, _params) do conn |> put_flash(:info, "Logged out successfully.") diff --git a/lib/demo_web/router.ex b/lib/demo_web/router.ex index ad1ef27..5b001b9 100644 --- a/lib/demo_web/router.ex +++ b/lib/demo_web/router.ex @@ -9,7 +9,6 @@ defmodule DemoWeb.Router do plug :fetch_flash plug :protect_from_forgery plug :put_secure_browser_headers - # NOTE: we may have to ask the user to do this one manually :/ plug :authenticate_user end From fe526856549ec60bae1fb3a736c0f5bbf9bf0342 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 24 Mar 2020 20:03:23 +0100 Subject: [PATCH 11/76] Initial batch of tests --- lib/demo/accounts/user.ex | 4 +- test/demo/accounts_test.exs | 158 ++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 test/demo/accounts_test.exs diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 8e6c7a3..cae33f8 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -29,7 +29,7 @@ defmodule Demo.Accounts.User do defp validate_email(changeset) do changeset |> validate_required([:email]) - |> validate_format(:email, "@") + |> validate_format(:email, ~r/^[^\s]+@[^\s]+$/, message: "must have the @ sign and no spaces") |> validate_length(:email, max: 160) |> unsafe_validate_unique(:email, Demo.Repo) |> unique_constraint(:email) @@ -94,7 +94,7 @@ defmodule Demo.Accounts.User do we encrypt a blank password to avoid timing attacks. """ def valid_password?(%Demo.Accounts.User{encrypted_password: encrypted_password}, password) - when is_binary(encrypted_password) do + when is_binary(encrypted_password) and byte_size(password) > 0 do Bcrypt.verify_pass(password, encrypted_password) end diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs new file mode 100644 index 0000000..38c06b2 --- /dev/null +++ b/test/demo/accounts_test.exs @@ -0,0 +1,158 @@ +defmodule Demo.AccountsTest do + use Demo.DataCase + + alias Demo.Accounts + alias Demo.Accounts.User + + @valid_password "hello world!" + + def user_fixture(attrs \\ %{}) do + {:ok, user} = + %{ + email: "user#{System.unique_integer()}@example.com", + password: @valid_password + } + |> Map.merge(Map.new(attrs)) + |> Accounts.register_user() + + user + end + + describe "get_user_by_email/1" do + test "does not return the user if the email does not exist" do + refute Accounts.get_user_by_email("unknown@example.com") + end + + test "returns the user if the email exists" do + %{id: id} = user = user_fixture() + assert %User{id: ^id} = Accounts.get_user_by_email(user.email) + end + end + + describe "get_user_by_email_nad_password/1" do + test "does not return the user if the email does not exist" do + refute Accounts.get_user_by_email_and_password("unknown@example.com", "hello world!") + end + + test "does not return the user if the password is not valid" do + user = user_fixture() + refute Accounts.get_user_by_email_and_password(user.email, "invalid") + end + + test "returns the user if the email and password are valid" do + %{id: id} = user = user_fixture() + assert %User{id: ^id} = Accounts.get_user_by_email_and_password(user.email, @valid_password) + end + end + + describe "get_user!/1" do + test "raises if id is invalid" do + assert_raise Ecto.NoResultsError, fn -> + Accounts.get_user!(123) + end + end + + test "returns the user with the given id" do + %{id: id} = user = user_fixture() + assert %User{id: ^id} = Accounts.get_user!(user.id) + end + end + + describe "register_user/1" do + test "requires email and password to be set" do + {:error, changeset} = Accounts.register_user(%{}) + + assert %{ + password: ["can't be blank"], + email: ["can't be blank"] + } = errors_on(changeset) + end + + test "validates email and password when given" do + {:error, changeset} = Accounts.register_user(%{email: "not valid", password: "not valid"}) + + assert %{ + email: ["must have the @ sign and no spaces"], + password: ["should be at least 12 character(s)"] + } = errors_on(changeset) + end + + test "validates maximum values for e-mail and password for security" do + too_long = String.duplicate("db", 100) + {:error, changeset} = Accounts.register_user(%{email: too_long, password: too_long}) + assert "should be at most 160 character(s)" in errors_on(changeset).email + assert "should be at most 80 character(s)" in errors_on(changeset).password + end + + test "validates e-mail uniqueness" do + %{email: email} = user_fixture() + {:error, changeset} = Accounts.register_user(%{email: email}) + assert "has already been taken" in errors_on(changeset).email + end + + test "registers users with an encrypted password" do + email = "user#{System.unique_integer()}@example.com" + {:ok, user} = Accounts.register_user(%{email: email, password: @valid_password}) + assert user.email == email + assert is_binary(user.encrypted_password) + assert is_nil(user.confirmed_at) + end + end + + describe "change_user_registration/2" do + test "returns a changeset" do + assert %Ecto.Changeset{} = changeset = Accounts.change_user_registration(%User{}) + assert changeset.required == [:password, :email] + end + end + + describe "change_user_email/2" do + test "returns a user changeset" do + assert %Ecto.Changeset{} = changeset = Accounts.change_user_email(%User{}) + assert changeset.required == [:email] + end + end + + describe "apply_user_email/3" do + setup do + %{user: user_fixture()} + end + + test "requires email to change", %{user: user} do + {:error, changeset} = Accounts.apply_user_email(user, @valid_password, %{}) + assert %{email: ["did not change"]} = errors_on(changeset) + end + + test "validates email", %{user: user} do + {:error, changeset} = + Accounts.apply_user_email(user, @valid_password, %{email: "not valid"}) + + assert %{email: ["must have the @ sign and no spaces"]} = errors_on(changeset) + end + + test "validates maximum values for e-mail and password for security", %{user: user} do + too_long = String.duplicate("db", 100) + {:error, changeset} = Accounts.apply_user_email(user, @valid_password, %{email: too_long}) + assert "should be at most 160 character(s)" in errors_on(changeset).email + end + + test "validates e-mail uniqueness", %{user: user} do + %{email: email} = user_fixture() + {:error, changeset} = Accounts.apply_user_email(user, @valid_password, %{email: email}) + assert "has already been taken" in errors_on(changeset).email + end + + test "validates current password", %{user: user} do + email = "user#{System.unique_integer()}@example.com" + {:error, changeset} = Accounts.apply_user_email(user, "invalid", %{email: email}) + assert %{current_password: ["is not valid"]} = errors_on(changeset) + end + + test "applies the e-mail without persisting it", %{user: user} do + email = "user#{System.unique_integer()}@example.com" + {:ok, user} = Accounts.apply_user_email(user, @valid_password, %{email: email}) + assert user.email == email + assert Accounts.get_user!(user.id).email != email + end + end +end From 3e30e6de2938e01d28422c66449344ca5c05a128 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 10:50:34 +0100 Subject: [PATCH 12/76] Always clear session on login/logout --- lib/demo_web/controllers/user_auth.ex | 41 +++++++++++++++++++-------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index c35d127..bac51e1 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -10,49 +10,66 @@ defmodule DemoWeb.UserAuth do # the token expiry itself in UserToken. @max_age 60 * 60 * 24 * 60 @remember_me_cookie "user_remember_me" + @remember_me_options [sign: true, max_age: @max_age] @doc """ Logs the user in. - It deletes the CSRF token and renews the session - to avoid fixation attacks. + It renews the session ID and clears the whole session + to avoid fixation attacks. See the renew_session + function to customize this behaviour. """ def login_user(conn, user, params \\ %{}) do - Plug.CSRFProtection.delete_csrf_token() token = Accounts.generate_session_token(user) - user_return_to = get_session(conn, :user_return_to) - delete_session(conn, :user_return_to) conn + |> renew_session() |> put_session(:user_token, token) - |> configure_session(renew: true) |> maybe_write_remember_me_cookie(token, params) |> redirect(to: user_return_to || signed_in_path(conn)) end defp maybe_write_remember_me_cookie(conn, token, %{"remember_me" => "true"}) do - put_resp_cookie(conn, @remember_me_cookie, token, sign: true, max_age: @max_age) + put_resp_cookie(conn, @remember_me_cookie, token, @remember_me_options) end defp maybe_write_remember_me_cookie(conn, _token, _params) do conn end + # This function renews the session ID and erases the whole + # session to avoid fixation attacks. If there is any data + # in the session you may want to preserve after login/logout, + # you must explicitly fetch the session data before clearing + # and then immediately set it after clearing, for example: + # + # def renew_session(conn) do + # preferred_locale = get_session(conn, :preferred_locale) + # + # conn + # |> configure_session(renew: true) + # |> clear_session() + # |> put_session(:preferred_locale, preferred_locale) + # end + # + defp renew_session(conn) do + conn + |> configure_session(renew: true) + |> clear_session() + end + @doc """ Logs the user out. - It clears all session data for safety. If you want to keep - some data in the session, we recommend you to manually copy - the data you want to maintain. + It clears all session data for safety. See renew_session. """ def logout_user(conn) do user_token = get_session(conn, :user_token) user_token && Accounts.delete_session_token(user_token) conn - |> clear_session() - |> configure_session(renew: true) + |> renew_session() |> delete_resp_cookie(@remember_me_cookie) |> redirect(to: "/") end From 5a2f0ed45f0b5b3008e2198a89adc670eea0cd49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 16:02:49 +0100 Subject: [PATCH 13/76] More tests --- test/demo/accounts_test.exs | 87 +++++++++++++++++++++++++++++++++---- 1 file changed, 78 insertions(+), 9 deletions(-) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 38c06b2..1680490 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -2,22 +2,31 @@ defmodule Demo.AccountsTest do use Demo.DataCase alias Demo.Accounts - alias Demo.Accounts.User + alias Demo.Accounts.{User, UserToken} @valid_password "hello world!" + def unique_email, do: "user#{System.unique_integer()}@example.com" + def user_fixture(attrs \\ %{}) do {:ok, user} = - %{ - email: "user#{System.unique_integer()}@example.com", - password: @valid_password - } + %{email: unique_email(), password: @valid_password} |> Map.merge(Map.new(attrs)) |> Accounts.register_user() user end + def capture_user_token(fun) do + captured = + ExUnit.CaptureIO.capture_io(fn -> + fun.(&"[TOKEN]#{&1}[TOKEN]") + end) + + [_, token, _] = String.split(captured, "[TOKEN]") + token + end + describe "get_user_by_email/1" do test "does not return the user if the email does not exist" do refute Accounts.get_user_by_email("unknown@example.com") @@ -91,7 +100,7 @@ defmodule Demo.AccountsTest do end test "registers users with an encrypted password" do - email = "user#{System.unique_integer()}@example.com" + email = unique_email() {:ok, user} = Accounts.register_user(%{email: email, password: @valid_password}) assert user.email == email assert is_binary(user.encrypted_password) @@ -143,16 +152,76 @@ defmodule Demo.AccountsTest do end test "validates current password", %{user: user} do - email = "user#{System.unique_integer()}@example.com" - {:error, changeset} = Accounts.apply_user_email(user, "invalid", %{email: email}) + {:error, changeset} = Accounts.apply_user_email(user, "invalid", %{email: unique_email()}) assert %{current_password: ["is not valid"]} = errors_on(changeset) end test "applies the e-mail without persisting it", %{user: user} do - email = "user#{System.unique_integer()}@example.com" + email = unique_email() {:ok, user} = Accounts.apply_user_email(user, @valid_password, %{email: email}) assert user.email == email assert Accounts.get_user!(user.id).email != email end end + + describe "deliver_update_email_instructions/3" do + setup do + %{user: user_fixture()} + end + + test "sends token through notification", %{user: user} do + token = + capture_user_token(fn url -> + assert Accounts.deliver_update_email_instructions(user, "current@example.com", url) == + :ok + end) + + {:ok, token} = Base.url_decode64(token, padding: false) + assert user_token = Repo.get_by(UserToken, token: :crypto.hash(:sha256, token)) + assert user_token.user_id == user.id + assert user_token.sent_to == user.email + assert user_token.context == "change:current@example.com" + end + end + + describe "update_user_email/2" do + setup do + user = user_fixture() + email = unique_email() + + token = + capture_user_token(fn url -> + Accounts.deliver_update_email_instructions(%{user | email: email}, user.email, url) + end) + + %{user: user, token: token, email: email} + end + + test "updates the e-mail with a valid token", %{user: user, token: token, email: email} do + assert Accounts.update_user_email(user, token) == :ok + changed_user = Repo.get!(User, user.id) + assert changed_user.email != user.email + assert changed_user.email == email + refute Repo.get_by(UserToken, user_id: user.id) + end + + test "does not update e-mail with invalid token", %{user: user} do + assert Accounts.update_user_email(user, "oops") == :error + assert Repo.get!(User, user.id).email == user.email + assert Repo.get_by(UserToken, user_id: user.id) + end + + test "does not update e-mail if user e-mail changed", %{user: user, token: token} do + assert Accounts.update_user_email(%{user | email: "current@example.com"}, token) == :error + assert Repo.get!(User, user.id).email == user.email + assert Repo.get_by(UserToken, user_id: user.id) + end + + test "does not update e-mail if token expired", %{user: user, token: token} do + {1, nil} = Repo.update_all(UserToken, set: [inserted_at: ~N[2020-01-01 00:00:00]]) + assert Accounts.update_user_email(user, token) == :error + assert Repo.get!(User, user.id).email == user.email + assert Repo.get_by(UserToken, user_id: user.id) + end + end end From d3b7a93d5bf0492275a540d23bdae33abfa69695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 16:06:45 +0100 Subject: [PATCH 14/76] change password context test --- test/demo/accounts_test.exs | 45 ++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 1680490..712f3d1 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -139,7 +139,7 @@ defmodule Demo.AccountsTest do assert %{email: ["must have the @ sign and no spaces"]} = errors_on(changeset) end - test "validates maximum values for e-mail and password for security", %{user: user} do + test "validates maximum value for e-mail for security", %{user: user} do too_long = String.duplicate("db", 100) {:error, changeset} = Accounts.apply_user_email(user, @valid_password, %{email: too_long}) assert "should be at most 160 character(s)" in errors_on(changeset).email @@ -224,4 +224,47 @@ defmodule Demo.AccountsTest do assert Repo.get_by(UserToken, user_id: user.id) end end + + describe "change_user_password/2" do + test "returns a user changeset" do + assert %Ecto.Changeset{} = changeset = Accounts.change_user_password(%User{}) + assert changeset.required == [:password] + end + end + + describe "update_user_password/3" do + setup do + %{user: user_fixture()} + end + + test "validates password", %{user: user} do + {:error, changeset} = + Accounts.update_user_password(user, @valid_password, %{password: "not valid"}) + + assert %{password: ["should be at least 12 character(s)"]} = errors_on(changeset) + end + + test "validates maximum values for password for security", %{user: user} do + too_long = String.duplicate("db", 100) + + {:error, changeset} = + Accounts.update_user_password(user, @valid_password, %{password: too_long}) + + assert "should be at most 80 character(s)" in errors_on(changeset).password + end + + test "validates current password", %{user: user} do + {:error, changeset} = + Accounts.update_user_password(user, "invalid", %{password: @valid_password}) + + assert %{current_password: ["is not valid"]} = errors_on(changeset) + end + + test "updates the password", %{user: user} do + {:ok, _} = + Accounts.update_user_password(user, @valid_password, %{password: "new valid password"}) + + assert Accounts.get_user_by_email_and_password(user.email, "new valid password") + end + end end From 653d4ed2660954b56b20bae0bb8db090c70c102e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 16:14:33 +0100 Subject: [PATCH 15/76] Session tests --- test/demo/accounts_test.exs | 43 +++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 712f3d1..eb284ba 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -267,4 +267,47 @@ defmodule Demo.AccountsTest do assert Accounts.get_user_by_email_and_password(user.email, "new valid password") end end + + describe "generate_session_token/1" do + setup do + %{user: user_fixture()} + end + + test "generates a token", %{user: user} do + token = Accounts.generate_session_token(user) + assert user_token = Repo.get_by(UserToken, token: token) + assert user_token.context == "session" + end + end + + describe "get_user_by_session_token/1" do + setup do + user = user_fixture() + token = Accounts.generate_session_token(user) + %{user: user, token: token} + end + + test "returns user by token", %{user: user, token: token} do + assert session_user = Accounts.get_user_by_session_token(token) + assert session_user.id == user.id + end + + test "does not return user for invalid token" do + refute Accounts.get_user_by_session_token("oops") + end + + test "does not return user for expired token", %{token: token} do + {1, nil} = Repo.update_all(UserToken, set: [inserted_at: ~N[2020-01-01 00:00:00]]) + refute Accounts.get_user_by_session_token(token) + end + end + + describe "delete_session_token/1" do + test "deletes the token" do + user = user_fixture() + token = Accounts.generate_session_token(user) + assert Accounts.delete_session_token(token) == :ok + refute Accounts.get_user_by_session_token("oops") + end + end end From ba4b09381c80dcd424417564c74bc239669b001b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 16:32:42 +0100 Subject: [PATCH 16/76] More tests and logic improvements --- lib/demo/accounts.ex | 32 +++++++---- lib/demo_web/controllers/user_confirmation.ex | 2 +- .../user_registration_controller.ex | 2 +- .../user_reset_password_controller.ex | 2 +- .../controllers/user_settings_controller.ex | 6 +- test/demo/accounts_test.exs | 56 +++++++++++++++++++ 6 files changed, 85 insertions(+), 15 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 9054c31..6799911 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -130,6 +130,7 @@ defmodule Demo.Accounts do Updates the user e-mail in token. If the token matches, the user email is updated and the token is deleted. + The confirmed_at date is also updated to the current time. """ def update_user_email(user, token) do context = "change:#{user.email}" @@ -144,8 +145,10 @@ defmodule Demo.Accounts do end defp user_email_multi(user, email, context) do + changeset = user |> User.email_changeset(%{email: email}) |> User.confirm_changeset() + Ecto.Multi.new() - |> Ecto.Multi.update(:user, User.email_changeset(user, %{email: email})) + |> Ecto.Multi.update(:user, changeset) |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, [context])) end @@ -194,10 +197,19 @@ defmodule Demo.Accounts do """ def update_user_password(user, password, attrs \\ %{}) do - user - |> User.password_changeset(attrs) - |> User.validate_current_password(password) - |> Repo.update() + changeset = + user + |> User.password_changeset(attrs) + |> User.validate_current_password(password) + + Ecto.Multi.new() + |> Ecto.Multi.update(:user, changeset) + |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, :all)) + |> Repo.transaction() + |> case do + {:ok, %{user: user}} -> {:ok, user} + {:error, :user, changeset, _} -> {:error, changeset} + end end ## Session @@ -234,14 +246,14 @@ defmodule Demo.Accounts do ## Examples - iex> deliver_confirmation_instructions(user, &Routes.user_confirmation_url(conn, :confirm)) + iex> deliver_user_confirmation_instructions(user, &Routes.user_confirmation_url(conn, :confirm)) :ok - iex> deliver_confirmation_instructions(confirmed_user, &Routes.user_confirmation_url(conn, :confirm)) + iex> deliver_user_confirmation_instructions(confirmed_user, &Routes.user_confirmation_url(conn, :confirm)) {:error, :already_confirmed} """ - def deliver_confirmation_instructions(%User{} = user, confirmation_url_fun) + def deliver_user_confirmation_instructions(%User{} = user, confirmation_url_fun) when is_function(confirmation_url_fun, 1) do if user.confirmed_at do {:error, :already_confirmed} @@ -282,11 +294,11 @@ defmodule Demo.Accounts do ## Examples - iex> deliver_reset_password_instructions(user, &Routes.user_reset_password_url(conn, :edit)) + iex> deliver_user_reset_password_instructions(user, &Routes.user_reset_password_url(conn, :edit)) :ok """ - def deliver_reset_password_instructions(%User{} = user, reset_password_url_fun) + def deliver_user_reset_password_instructions(%User{} = user, reset_password_url_fun) when is_function(reset_password_url_fun, 1) do {encoded_token, user_token} = UserToken.build_user_email_token(user, "reset_password") Repo.insert!(user_token) diff --git a/lib/demo_web/controllers/user_confirmation.ex b/lib/demo_web/controllers/user_confirmation.ex index 2e1d621..9d4eec6 100644 --- a/lib/demo_web/controllers/user_confirmation.ex +++ b/lib/demo_web/controllers/user_confirmation.ex @@ -9,7 +9,7 @@ defmodule DemoWeb.UserConfirmationController do def create(conn, %{"user" => %{"email" => email}}) do if user = Accounts.get_user_by_email(email) do - Accounts.deliver_confirmation_instructions( + Accounts.deliver_user_confirmation_instructions( user, &Routes.user_confirmation_url(conn, :confirm, &1) ) diff --git a/lib/demo_web/controllers/user_registration_controller.ex b/lib/demo_web/controllers/user_registration_controller.ex index b824a89..4d762a8 100644 --- a/lib/demo_web/controllers/user_registration_controller.ex +++ b/lib/demo_web/controllers/user_registration_controller.ex @@ -14,7 +14,7 @@ defmodule DemoWeb.UserRegistrationController do case Accounts.register_user(user_params) do {:ok, user} -> :ok = - Accounts.deliver_confirmation_instructions( + Accounts.deliver_user_confirmation_instructions( user, &Routes.user_confirmation_url(conn, :confirm, &1) ) diff --git a/lib/demo_web/controllers/user_reset_password_controller.ex b/lib/demo_web/controllers/user_reset_password_controller.ex index 06d1e52..087113b 100644 --- a/lib/demo_web/controllers/user_reset_password_controller.ex +++ b/lib/demo_web/controllers/user_reset_password_controller.ex @@ -11,7 +11,7 @@ defmodule DemoWeb.UserResetPasswordController do def create(conn, %{"user" => %{"email" => email}}) do if user = Accounts.get_user_by_email(email) do - Accounts.deliver_reset_password_instructions( + Accounts.deliver_user_reset_password_instructions( user, &Routes.user_reset_password_url(conn, :edit, &1) ) diff --git a/lib/demo_web/controllers/user_settings_controller.ex b/lib/demo_web/controllers/user_settings_controller.ex index 799647c..649140b 100644 --- a/lib/demo_web/controllers/user_settings_controller.ex +++ b/lib/demo_web/controllers/user_settings_controller.ex @@ -2,6 +2,7 @@ defmodule DemoWeb.UserSettingsController do use DemoWeb, :controller alias Demo.Accounts + alias DemoWeb.UserAuth plug :assign_email_and_password_changesets @@ -50,10 +51,11 @@ defmodule DemoWeb.UserSettingsController do user = conn.assigns.current_user case Accounts.update_user_password(user, password, user_params) do - {:ok, _} -> + {:ok, user} -> conn |> put_flash(:info, "Password updated successfully.") - |> redirect(to: Routes.user_settings_path(conn, :edit)) + |> put_session(:user_return_to, Routes.user_settings_path(conn, :edit)) + |> UserAuth.login_user(user) {:error, changeset} -> render(conn, "edit.html", password_changeset: changeset) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index eb284ba..ea1d1cd 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -202,6 +202,8 @@ defmodule Demo.AccountsTest do changed_user = Repo.get!(User, user.id) assert changed_user.email != user.email assert changed_user.email == email + assert changed_user.confirmed_at + assert changed_user.confirmed_at != user.confirmed_at refute Repo.get_by(UserToken, user_id: user.id) end @@ -310,4 +312,58 @@ defmodule Demo.AccountsTest do refute Accounts.get_user_by_session_token("oops") end end + + describe "deliver_user_confirmation_instructions/3" do + setup do + %{user: user_fixture()} + end + + test "sends token through notification", %{user: user} do + token = + capture_user_token(fn url -> + assert Accounts.deliver_user_confirmation_instructions(user, url) == + :ok + end) + + {:ok, token} = Base.url_decode64(token, padding: false) + assert user_token = Repo.get_by(UserToken, token: :crypto.hash(:sha256, token)) + assert user_token.user_id == user.id + assert user_token.sent_to == user.email + assert user_token.context == "confirm" + end + end + + describe "confirm_user/2" do + setup do + user = user_fixture() + + token = + capture_user_token(fn url -> + Accounts.deliver_user_confirmation_instructions(user, url) + end) + + %{user: user, token: token} + end + + test "confirms the e-mail with a valid token", %{user: user, token: token} do + assert Accounts.confirm_user(token) == :ok + changed_user = Repo.get!(User, user.id) + assert changed_user.confirmed_at + assert changed_user.confirmed_at != user.confirmed_at + refute Repo.get_by(UserToken, user_id: user.id) + end + + test "does not confirm with invalid token", %{user: user} do + assert Accounts.confirm_user("oops") == :error + refute Repo.get!(User, user.id).confirmed_at + assert Repo.get_by(UserToken, user_id: user.id) + end + + test "does not update e-mail if token expired", %{user: user, token: token} do + {1, nil} = Repo.update_all(UserToken, set: [inserted_at: ~N[2020-01-01 00:00:00]]) + assert Accounts.confirm_user(token) == :error + refute Repo.get!(User, user.id).confirmed_at + assert Repo.get_by(UserToken, user_id: user.id) + end + end end From c157742348baa90b0342f1f116e6b3602c0d51b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 16:54:57 +0100 Subject: [PATCH 17/76] Domain tests done --- lib/demo/accounts.ex | 11 ++-- test/demo/accounts_test.exs | 108 ++++++++++++++++++++++++++++++++++-- 2 files changed, 109 insertions(+), 10 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 6799911..f18bcdc 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -72,7 +72,7 @@ defmodule Demo.Accounts do {:error, %Ecto.Changeset{}} """ - def register_user(attrs \\ %{}) do + def register_user(attrs) do %User{} |> User.registration_changeset(attrs) |> Repo.insert() @@ -196,7 +196,7 @@ defmodule Demo.Accounts do {:error, %Ecto.Changeset{}} """ - def update_user_password(user, password, attrs \\ %{}) do + def update_user_password(user, password, attrs) do changeset = user |> User.password_changeset(attrs) @@ -311,10 +311,10 @@ defmodule Demo.Accounts do ## Examples - iex> get_user_by_reset_password_token("validtoken-sadsadsa") + iex> get_user_by_reset_password_token("validtoken") %User{} - iex> get_user_by_reset_password_token("invalidtoken-sadsadsa") + iex> get_user_by_reset_password_token("invalidtoken") nil """ @@ -332,7 +332,6 @@ defmodule Demo.Accounts do ## Examples - iex> reset_user_password(user, %{password: "new long password", password_confirmation: "new long password"}) {:ok, %User{}} @@ -340,7 +339,7 @@ defmodule Demo.Accounts do {:error, %Ecto.Changeset{}} """ - def reset_user_password(user, attrs \\ %{}) do + def reset_user_password(user, attrs) do Ecto.Multi.new() |> Ecto.Multi.update(:user, User.password_changeset(user, attrs)) |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, :all)) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index ea1d1cd..1d3c8ed 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -241,9 +241,15 @@ defmodule Demo.AccountsTest do test "validates password", %{user: user} do {:error, changeset} = - Accounts.update_user_password(user, @valid_password, %{password: "not valid"}) + Accounts.update_user_password(user, @valid_password, %{ + password: "not valid", + password_confirmation: "another" + }) - assert %{password: ["should be at least 12 character(s)"]} = errors_on(changeset) + assert %{ + password: ["should be at least 12 character(s)"], + password_confirmation: ["does not match confirmation"] + } = errors_on(changeset) end test "validates maximum values for password for security", %{user: user} do @@ -268,6 +274,15 @@ defmodule Demo.AccountsTest do assert Accounts.get_user_by_email_and_password(user.email, "new valid password") end + + test "deletes all tokens for the given user", %{user: user} do + _ = Accounts.generate_session_token(user) + + {:ok, _} = + Accounts.update_user_password(user, @valid_password, %{password: "new valid password"}) + + refute Repo.get_by(UserToken, user_id: user.id) + end end describe "generate_session_token/1" do @@ -313,7 +328,7 @@ defmodule Demo.AccountsTest do end end - describe "deliver_user_confirmation_instructions/3" do + describe "deliver_user_confirmation_instructions/2" do setup do %{user: user_fixture()} end @@ -359,11 +374,96 @@ defmodule Demo.AccountsTest do assert Repo.get_by(UserToken, user_id: user.id) end - test "does not update e-mail if token expired", %{user: user, token: token} do + test "does not confirm e-mail if token expired", %{user: user, token: token} do {1, nil} = Repo.update_all(UserToken, set: [inserted_at: ~N[2020-01-01 00:00:00]]) assert Accounts.confirm_user(token) == :error refute Repo.get!(User, user.id).confirmed_at assert Repo.get_by(UserToken, user_id: user.id) end end + + describe "deliver_user_reset_password_instructions/2" do + setup do + %{user: user_fixture()} + end + + test "sends token through notification", %{user: user} do + token = + capture_user_token(fn url -> + assert Accounts.deliver_user_reset_password_instructions(user, url) == + :ok + end) + + {:ok, token} = Base.url_decode64(token, padding: false) + assert user_token = Repo.get_by(UserToken, token: :crypto.hash(:sha256, token)) + assert user_token.user_id == user.id + assert user_token.sent_to == user.email + assert user_token.context == "reset_password" + end + end + + describe "get_user_by_reset_password_token/2" do + setup do + user = user_fixture() + + token = + capture_user_token(fn url -> + Accounts.deliver_user_reset_password_instructions(user, url) + end) + + %{user: user, token: token} + end + + test "returns the user with valid token", %{user: %{id: id}, token: token} do + assert %User{id: ^id} = Accounts.get_user_by_reset_password_token(token) + assert Repo.get_by(UserToken, user_id: id) + end + + test "does not return the user with invalid token", %{user: user} do + refute Accounts.get_user_by_reset_password_token("oops") + assert Repo.get_by(UserToken, user_id: user.id) + end + + test "does not return the user if token expired", %{user: user, token: token} do + {1, nil} = Repo.update_all(UserToken, set: [inserted_at: ~N[2020-01-01 00:00:00]]) + refute Accounts.get_user_by_reset_password_token(token) + assert Repo.get_by(UserToken, user_id: user.id) + end + end + + describe "reset_user_password/3" do + setup do + %{user: user_fixture()} + end + + test "validates password", %{user: user} do + {:error, changeset} = + Accounts.reset_user_password(user, %{ + password: "not valid", + password_confirmation: "another" + }) + + assert %{ + password: ["should be at least 12 character(s)"], + password_confirmation: ["does not match confirmation"] + } = errors_on(changeset) + end + + test "validates maximum values for password for security", %{user: user} do + too_long = String.duplicate("db", 100) + {:error, changeset} = Accounts.reset_user_password(user, %{password: too_long}) + assert "should be at most 80 character(s)" in errors_on(changeset).password + end + + test "updates the password", %{user: user} do + {:ok, _} = Accounts.reset_user_password(user, %{password: "new valid password"}) + assert Accounts.get_user_by_email_and_password(user.email, "new valid password") + end + + test "deletes all tokens for the given user", %{user: user} do + _ = Accounts.generate_session_token(user) + {:ok, _} = Accounts.reset_user_password(user, %{password: "new valid password"}) + refute Repo.get_by(UserToken, user_id: user.id) + end + end end From f137822adc1ba66a034027a64f0228b7a128c025 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 20:08:27 +0100 Subject: [PATCH 18/76] UserAuth tests --- lib/demo_web/controllers/user_auth.ex | 6 +- lib/demo_web/router.ex | 2 +- mix.lock | 8 +- test/demo/accounts_test.exs | 70 +++++----- test/demo_web/controllers/user_auth_test.exs | 130 +++++++++++++++++++ test/support/fixtures/accounts_fixtures.ex | 26 ++++ 6 files changed, 195 insertions(+), 47 deletions(-) create mode 100644 test/demo_web/controllers/user_auth_test.exs create mode 100644 test/support/fixtures/accounts_fixtures.ex diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index bac51e1..1698c43 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -78,7 +78,7 @@ defmodule DemoWeb.UserAuth do Authenticates the user by looking into the session and remember me token. """ - def authenticate_user(conn, _opts) do + def fetch_current_user(conn, _opts) do {user_token, conn} = ensure_user_token(conn) user = user_token && Accounts.get_user_by_session_token(user_token) assign(conn, :current_user, user) @@ -90,7 +90,7 @@ defmodule DemoWeb.UserAuth do else conn = fetch_cookies(conn, signed: [@remember_me_cookie]) - if user_token = conn.req_cookies[@remember_me_cookie] do + if user_token = conn.cookies[@remember_me_cookie] do {user_token, put_session(conn, :user_token, user_token)} else {nil, conn} @@ -122,7 +122,7 @@ defmodule DemoWeb.UserAuth do conn else conn - |> put_flash(:error, "You must be authenticated to access this page.") + |> put_flash(:error, "You must login to access this page.") |> put_session(:user_return_to, conn.request_path) |> redirect(to: Routes.user_session_path(conn, :new)) |> halt() diff --git a/lib/demo_web/router.ex b/lib/demo_web/router.ex index 5b001b9..f859c7a 100644 --- a/lib/demo_web/router.ex +++ b/lib/demo_web/router.ex @@ -9,7 +9,7 @@ defmodule DemoWeb.Router do plug :fetch_flash plug :protect_from_forgery plug :put_secure_browser_headers - plug :authenticate_user + plug :fetch_current_user end pipeline :api do diff --git a/mix.lock b/mix.lock index 4659bf8..622cee3 100644 --- a/mix.lock +++ b/mix.lock @@ -6,14 +6,14 @@ "cowlib": {:hex, :cowlib, "2.8.0", "fd0ff1787db84ac415b8211573e9a30a3ebe71b5cbff7f720089972b2319c8a4", [:rebar3], [], "hexpm", "79f954a7021b302186a950a32869dbc185523d99d3e44ce430cd1f3289f41ed4"}, "db_connection": {:hex, :db_connection, "2.2.1", "caee17725495f5129cb7faebde001dc4406796f12a62b8949f4ac69315080566", [:mix], [{:connection, "~> 1.0.2", [hex: :connection, repo: "hexpm", optional: false]}], "hexpm", "2b02ece62d9f983fcd40954e443b7d9e6589664380e5546b2b9b523cd0fb59e1"}, "decimal": {:hex, :decimal, "1.8.1", "a4ef3f5f3428bdbc0d35374029ffcf4ede8533536fa79896dd450168d9acdf3c", [:mix], [], "hexpm", "3cb154b00225ac687f6cbd4acc4b7960027c757a5152b369923ead9ddbca7aec"}, - "ecto": {:hex, :ecto, "3.3.4", "95b05c82ae91361475e5491c9f3ac47632f940b3f92ae3988ac1aad04989c5bb", [:mix], [{:decimal, "~> 1.6 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}], "hexpm", "9b96cbb83a94713731461ea48521b178b0e3863d310a39a3948c807266eebd69"}, - "ecto_sql": {:hex, :ecto_sql, "3.3.4", "aa18af12eb875fbcda2f75e608b3bd534ebf020fc4f6448e4672fcdcbb081244", [:mix], [{:db_connection, "~> 2.2", [hex: :db_connection, repo: "hexpm", optional: false]}, {:ecto, "~> 3.4 or ~> 3.3.3", [hex: :ecto, repo: "hexpm", optional: false]}, {:myxql, "~> 0.3.0", [hex: :myxql, repo: "hexpm", optional: true]}, {:postgrex, "~> 0.15.0", [hex: :postgrex, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "5eccbdbf92e3c6f213007a82d5dbba4cd9bb659d1a21331f89f408e4c0efd7a8"}, + "ecto": {:hex, :ecto, "3.4.0", "a7a83ab8359bf816ce729e5e65981ce25b9fc5adfc89c2ea3980f4fed0bfd7c1", [:mix], [{:decimal, "~> 1.6 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}], "hexpm", "5eed18252f5b5bbadec56a24112b531343507dbe046273133176b12190ce19cc"}, + "ecto_sql": {:hex, :ecto_sql, "3.4.1", "3c9136ba138f9b74d31286c73c61232a92bd19385f7c5607bdeb3a4587ef91f5", [:mix], [{:db_connection, "~> 2.2", [hex: :db_connection, repo: "hexpm", optional: false]}, {:ecto, "~> 3.4.0", [hex: :ecto, repo: "hexpm", optional: false]}, {:myxql, "~> 0.3.0 or ~> 0.4.0", [hex: :myxql, repo: "hexpm", optional: true]}, {:postgrex, "~> 0.15.0", [hex: :postgrex, repo: "hexpm", optional: true]}, {:tds, "~> 2.1.0", [hex: :tds, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "9b4be0bffe7b0bdf5393defcae52712f248e70cc2bc0e8ab6ddb03be66371516"}, "elixir_make": {:hex, :elixir_make, "0.6.0", "38349f3e29aff4864352084fc736fa7fa0f2995a819a737554f7ebd28b85aaab", [:mix], [], "hexpm", "d522695b93b7f0b4c0fcb2dfe73a6b905b1c301226a5a55cb42e5b14d509e050"}, "file_system": {:hex, :file_system, "0.2.8", "f632bd287927a1eed2b718f22af727c5aeaccc9a98d8c2bd7bff709e851dc986", [:mix], [], "hexpm", "97a3b6f8d63ef53bd0113070102db2ce05352ecf0d25390eb8d747c2bde98bca"}, "gettext": {:hex, :gettext, "0.17.4", "f13088e1ec10ce01665cf25f5ff779e7df3f2dc71b37084976cf89d1aa124d5c", [:mix], [], "hexpm", "3c75b5ea8288e2ee7ea503ff9e30dfe4d07ad3c054576a6e60040e79a801e14d"}, - "jason": {:hex, :jason, "1.1.2", "b03dedea67a99223a2eaf9f1264ce37154564de899fd3d8b9a21b1a6fd64afe7", [:mix], [{:decimal, "~> 1.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "fdf843bca858203ae1de16da2ee206f53416bbda5dc8c9e78f43243de4bc3afe"}, + "jason": {:hex, :jason, "1.2.0", "10043418c42d2493d0ee212d3fddd25d7ffe484380afad769a0a38795938e448", [:mix], [{:decimal, "~> 1.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "116747dbe057794c3a3e4e143b7c8390b29f634e16c78a7f59ba75bfa6852e7f"}, "mime": {:hex, :mime, "1.3.1", "30ce04ab3175b6ad0bdce0035cba77bba68b813d523d1aac73d9781b4d193cf8", [:mix], [], "hexpm", "6cbe761d6a0ca5a31a0931bf4c63204bceb64538e664a8ecf784a9a6f3b875f1"}, - "phoenix": {:git, "https://github.com/phoenixframework/phoenix.git", "17fa0596aac4ec1e2ab6542c2c022cdf9a75d852", []}, + "phoenix": {:git, "https://github.com/phoenixframework/phoenix.git", "7c1bffcdfceb638da832b845aa790005ce5681ed", []}, "phoenix_ecto": {:hex, :phoenix_ecto, "4.1.0", "a044d0756d0464c5a541b4a0bf4bcaf89bffcaf92468862408290682c73ae50d", [:mix], [{:ecto, "~> 3.0", [hex: :ecto, repo: "hexpm", optional: false]}, {:phoenix_html, "~> 2.9", [hex: :phoenix_html, repo: "hexpm", optional: true]}, {:plug, "~> 1.0", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "c5e666a341ff104d0399d8f0e4ff094559b2fde13a5985d4cb5023b2c2ac558b"}, "phoenix_html": {:hex, :phoenix_html, "2.14.0", "d8c6bc28acc8e65f8ea0080ee05aa13d912c8758699283b8d3427b655aabe284", [:mix], [{:plug, "~> 1.5", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "b0bb30eda478a06dbfbe96728061a93833db3861a49ccb516f839ecb08493fbb"}, "phoenix_live_reload": {:hex, :phoenix_live_reload, "1.2.1", "274a4b07c4adbdd7785d45a8b0bb57634d0b4f45b18d2c508b26c0344bd59b8f", [:mix], [{:file_system, "~> 0.2.1 or ~> 0.3", [hex: :file_system, repo: "hexpm", optional: false]}, {:phoenix, "~> 1.4", [hex: :phoenix, repo: "hexpm", optional: false]}], "hexpm", "41b4103a2fa282cfd747d377233baf213c648fdcc7928f432937676532490eee"}, diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 1d3c8ed..79b906e 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -1,32 +1,10 @@ defmodule Demo.AccountsTest do use Demo.DataCase + import Demo.AccountsFixtures alias Demo.Accounts alias Demo.Accounts.{User, UserToken} - @valid_password "hello world!" - - def unique_email, do: "user#{System.unique_integer()}@example.com" - - def user_fixture(attrs \\ %{}) do - {:ok, user} = - %{email: unique_email(), password: @valid_password} - |> Map.merge(Map.new(attrs)) - |> Accounts.register_user() - - user - end - - def capture_user_token(fun) do - captured = - ExUnit.CaptureIO.capture_io(fn -> - fun.(&"[TOKEN]#{&1}[TOKEN]") - end) - - [_, token, _] = String.split(captured, "[TOKEN]") - token - end - describe "get_user_by_email/1" do test "does not return the user if the email does not exist" do refute Accounts.get_user_by_email("unknown@example.com") @@ -50,7 +28,9 @@ defmodule Demo.AccountsTest do test "returns the user if the email and password are valid" do %{id: id} = user = user_fixture() - assert %User{id: ^id} = Accounts.get_user_by_email_and_password(user.email, @valid_password) + + assert %User{id: ^id} = + Accounts.get_user_by_email_and_password(user.email, valid_user_password()) end end @@ -100,8 +80,8 @@ defmodule Demo.AccountsTest do end test "registers users with an encrypted password" do - email = unique_email() - {:ok, user} = Accounts.register_user(%{email: email, password: @valid_password}) + email = unique_user_email() + {:ok, user} = Accounts.register_user(%{email: email, password: valid_user_password()}) assert user.email == email assert is_binary(user.encrypted_password) assert is_nil(user.confirmed_at) @@ -128,37 +108,45 @@ defmodule Demo.AccountsTest do end test "requires email to change", %{user: user} do - {:error, changeset} = Accounts.apply_user_email(user, @valid_password, %{}) + {:error, changeset} = Accounts.apply_user_email(user, valid_user_password(), %{}) assert %{email: ["did not change"]} = errors_on(changeset) end test "validates email", %{user: user} do {:error, changeset} = - Accounts.apply_user_email(user, @valid_password, %{email: "not valid"}) + Accounts.apply_user_email(user, valid_user_password(), %{email: "not valid"}) assert %{email: ["must have the @ sign and no spaces"]} = errors_on(changeset) end test "validates maximum value for e-mail for security", %{user: user} do too_long = String.duplicate("db", 100) - {:error, changeset} = Accounts.apply_user_email(user, @valid_password, %{email: too_long}) + + {:error, changeset} = + Accounts.apply_user_email(user, valid_user_password(), %{email: too_long}) + assert "should be at most 160 character(s)" in errors_on(changeset).email end test "validates e-mail uniqueness", %{user: user} do %{email: email} = user_fixture() - {:error, changeset} = Accounts.apply_user_email(user, @valid_password, %{email: email}) + + {:error, changeset} = + Accounts.apply_user_email(user, valid_user_password(), %{email: email}) + assert "has already been taken" in errors_on(changeset).email end test "validates current password", %{user: user} do - {:error, changeset} = Accounts.apply_user_email(user, "invalid", %{email: unique_email()}) + {:error, changeset} = + Accounts.apply_user_email(user, "invalid", %{email: unique_user_email()}) + assert %{current_password: ["is not valid"]} = errors_on(changeset) end test "applies the e-mail without persisting it", %{user: user} do - email = unique_email() - {:ok, user} = Accounts.apply_user_email(user, @valid_password, %{email: email}) + email = unique_user_email() + {:ok, user} = Accounts.apply_user_email(user, valid_user_password(), %{email: email}) assert user.email == email assert Accounts.get_user!(user.id).email != email end @@ -187,7 +175,7 @@ defmodule Demo.AccountsTest do describe "update_user_email/2" do setup do user = user_fixture() - email = unique_email() + email = unique_user_email() token = capture_user_token(fn url -> @@ -241,7 +229,7 @@ defmodule Demo.AccountsTest do test "validates password", %{user: user} do {:error, changeset} = - Accounts.update_user_password(user, @valid_password, %{ + Accounts.update_user_password(user, valid_user_password(), %{ password: "not valid", password_confirmation: "another" }) @@ -256,21 +244,23 @@ defmodule Demo.AccountsTest do too_long = String.duplicate("db", 100) {:error, changeset} = - Accounts.update_user_password(user, @valid_password, %{password: too_long}) + Accounts.update_user_password(user, valid_user_password(), %{password: too_long}) assert "should be at most 80 character(s)" in errors_on(changeset).password end test "validates current password", %{user: user} do {:error, changeset} = - Accounts.update_user_password(user, "invalid", %{password: @valid_password}) + Accounts.update_user_password(user, "invalid", %{password: valid_user_password()}) assert %{current_password: ["is not valid"]} = errors_on(changeset) end test "updates the password", %{user: user} do {:ok, _} = - Accounts.update_user_password(user, @valid_password, %{password: "new valid password"}) + Accounts.update_user_password(user, valid_user_password(), %{ + password: "new valid password" + }) assert Accounts.get_user_by_email_and_password(user.email, "new valid password") end @@ -279,7 +269,9 @@ defmodule Demo.AccountsTest do _ = Accounts.generate_session_token(user) {:ok, _} = - Accounts.update_user_password(user, @valid_password, %{password: "new valid password"}) + Accounts.update_user_password(user, valid_user_password(), %{ + password: "new valid password" + }) refute Repo.get_by(UserToken, user_id: user.id) end diff --git a/test/demo_web/controllers/user_auth_test.exs b/test/demo_web/controllers/user_auth_test.exs new file mode 100644 index 0000000..b43746f --- /dev/null +++ b/test/demo_web/controllers/user_auth_test.exs @@ -0,0 +1,130 @@ +defmodule DemoWeb.UserAuthTest do + use DemoWeb.ConnCase + + alias Demo.Accounts + alias DemoWeb.UserAuth + import Demo.AccountsFixtures + + setup %{conn: conn} do + conn = + conn + |> Map.replace!(:secret_key_base, DemoWeb.Endpoint.config(:secret_key_base)) + |> init_test_session(%{}) + + %{user: user_fixture(), conn: conn} + end + + describe "login_user/3" do + test "stores the user token in the session", %{conn: conn, user: user} do + conn = UserAuth.login_user(conn, user) + assert token = get_session(conn, :user_token) + assert redirected_to(conn) == "/" + assert Accounts.get_user_by_session_token(token) + end + + test "clears everything previously stored in the session", %{conn: conn, user: user} do + conn = conn |> put_session(:to_be_removed, "value") |> UserAuth.login_user(user) + refute get_session(conn, :to_be_removed) + end + + test "redirects to the configured path", %{conn: conn, user: user} do + conn = conn |> put_session(:user_return_to, "/hello") |> UserAuth.login_user(user) + assert redirected_to(conn) == "/hello" + end + + test "writes a cookie if remember_me is configured", %{conn: conn, user: user} do + conn = conn |> fetch_cookies() |> UserAuth.login_user(user, %{"remember_me" => "true"}) + assert get_session(conn, :user_token) == conn.cookies["user_remember_me"] + + assert %{value: signed_token, max_age: max_age} = conn.resp_cookies["user_remember_me"] + assert signed_token != get_session(conn, :user_token) + assert max_age == 5_184_000 + end + end + + describe "logout_user/1" do + test "erases session and cookies", %{conn: conn, user: user} do + user_token = Accounts.generate_session_token(user) + + conn = + conn + |> put_session(:user_token, user_token) + |> put_req_cookie("user_remember_me", user_token) + |> fetch_cookies() + |> UserAuth.logout_user() + + refute get_session(conn, :user_token) + refute conn.cookies["user_remember_me"] + assert %{max_age: 0} = conn.resp_cookies["user_remember_me"] + assert redirected_to(conn) == "/" + refute Accounts.get_user_by_session_token(user_token) + end + + test "works even if user is already logged out", %{conn: conn} do + conn = conn |> fetch_cookies() |> UserAuth.logout_user() + refute get_session(conn, :user_token) + assert %{max_age: 0} = conn.resp_cookies["user_remember_me"] + assert redirected_to(conn) == "/" + end + end + + describe "fetch_current_user/2" do + test "authenticates user from session", %{conn: conn, user: user} do + user_token = Accounts.generate_session_token(user) + conn = conn |> put_session(:user_token, user_token) |> UserAuth.fetch_current_user([]) + assert conn.assigns.current_user.id == user.id + end + + test "authenticates user from cookies", %{conn: conn, user: user} do + logged_in_conn = + conn |> fetch_cookies() |> UserAuth.login_user(user, %{"remember_me" => "true"}) + + user_token = logged_in_conn.cookies["user_remember_me"] + %{value: signed_token} = logged_in_conn.resp_cookies["user_remember_me"] + + conn = + conn + |> put_req_cookie("user_remember_me", signed_token) + |> UserAuth.fetch_current_user([]) + + assert get_session(conn, :user_token) == user_token + assert conn.assigns.current_user.id == user.id + end + + test "does not authenticate if data is missing", %{conn: conn, user: user} do + _ = Accounts.generate_session_token(user) + conn = UserAuth.fetch_current_user(conn, []) + refute get_session(conn, :user_token) + refute conn.assigns.current_user + end + end + + describe "redirect_if_user_is_authenticated/2" do + test "redirects if user is authenticated", %{conn: conn, user: user} do + conn = conn |> assign(:current_user, user) |> UserAuth.redirect_if_user_is_authenticated([]) + assert conn.halted + assert redirected_to(conn) == "/" + end + + test "does not redirect if user is not authenticated", %{conn: conn} do + conn = UserAuth.redirect_if_user_is_authenticated(conn, []) + refute conn.halted + refute conn.status + end + end + + describe "require_authenticated_user/2" do + test "redirects if user is not authenticated", %{conn: conn} do + conn = conn |> fetch_flash() |> UserAuth.require_authenticated_user([]) + assert conn.halted + assert redirected_to(conn) == "/users/login" + assert get_flash(conn, :error) == "You must login to access this page." + end + + test "does not redirect if user is not authenticated", %{conn: conn, user: user} do + conn = conn |> assign(:current_user, user) |> UserAuth.require_authenticated_user([]) + refute conn.halted + refute conn.status + end + end +end diff --git a/test/support/fixtures/accounts_fixtures.ex b/test/support/fixtures/accounts_fixtures.ex new file mode 100644 index 0000000..7a309a6 --- /dev/null +++ b/test/support/fixtures/accounts_fixtures.ex @@ -0,0 +1,26 @@ +defmodule Demo.AccountsFixtures do + def unique_user_email, do: "user#{System.unique_integer()}@example.com" + def valid_user_password, do: "hello world!" + + def user_fixture(attrs \\ %{}) do + {:ok, user} = + attrs + |> Enum.into(%{ + email: unique_user_email(), + password: valid_user_password() + }) + |> Demo.Accounts.register_user() + + user + end + + def capture_user_token(fun) do + captured = + ExUnit.CaptureIO.capture_io(fn -> + fun.(&"[TOKEN]#{&1}[TOKEN]") + end) + + [_, token, _] = String.split(captured, "[TOKEN]") + token + end +end From 5b550b8520880d92e1b832113d457d6b0d073bbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 20:14:35 +0100 Subject: [PATCH 19/76] Update test/demo/accounts_test.exs Co-Authored-By: Aaron Renner --- test/demo/accounts_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 79b906e..bcd5834 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -16,7 +16,7 @@ defmodule Demo.AccountsTest do end end - describe "get_user_by_email_nad_password/1" do + describe "get_user_by_email_and_password/1" do test "does not return the user if the email does not exist" do refute Accounts.get_user_by_email_and_password("unknown@example.com", "hello world!") end From e7dd9e12b75c76cbb9330afc2752686686de8fd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 23:47:39 +0100 Subject: [PATCH 20/76] Update lib/demo/accounts.ex Co-Authored-By: Aaron Renner --- lib/demo/accounts.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index f18bcdc..2c4a190 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -16,7 +16,7 @@ defmodule Demo.Accounts do iex> get_user_by_email("foo@example.com") %User{} - iex> get_user_by_emai("unknown@example.com") + iex> get_user_by_email("unknown@example.com") nil """ From 4e024615adbcfc53112aa43fde590321775d13bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 25 Mar 2020 23:47:47 +0100 Subject: [PATCH 21/76] Update lib/demo/accounts.ex Co-Authored-By: Aaron Renner --- lib/demo/accounts.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 2c4a190..6e9adf5 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -287,7 +287,7 @@ defmodule Demo.Accounts do |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, ["confirm"])) end - ## Reset passwword + ## Reset password @doc """ Delivers the reset password e-mail to the given user. From 39ccccb1ca471fa7aaf70b773f343f3fb8face73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 10:48:02 +0100 Subject: [PATCH 22/76] Initial controller tests --- .../user_session_controller_test.exs | 21 +++++++++++++++++++ test/support/conn_case.ex | 10 +++++++++ 2 files changed, 31 insertions(+) create mode 100644 test/demo_web/controllers/user_session_controller_test.exs diff --git a/test/demo_web/controllers/user_session_controller_test.exs b/test/demo_web/controllers/user_session_controller_test.exs new file mode 100644 index 0000000..c5f0d9e --- /dev/null +++ b/test/demo_web/controllers/user_session_controller_test.exs @@ -0,0 +1,21 @@ +defmodule DemoWeb.UserSessionControllerTest do + use DemoWeb.ConnCase + + import Demo.AccountsFixtures + + setup do + %{user: user_fixture()} + end + + describe "GET /users/login" do + test "renders login page", %{conn: conn} do + conn = get(conn, Routes.user_session_path(conn, :new)) + assert html_response(conn, 200) =~ "

    Login

    " + end + + test "redirects if already logged in", %{conn: conn, user: user} do + conn = conn |> login_user(user) |> get(Routes.user_session_path(conn, :new)) + assert redirected_to(conn) == "/" + end + end +end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 19c5857..39e0835 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -22,6 +22,8 @@ defmodule DemoWeb.ConnCase do # Import conveniences for testing with connections import Plug.Conn import Phoenix.ConnTest + import DemoWeb.ConnCase + alias DemoWeb.Router.Helpers, as: Routes # The default endpoint for testing @@ -38,4 +40,12 @@ defmodule DemoWeb.ConnCase do {:ok, conn: Phoenix.ConnTest.build_conn()} end + + def login_user(conn, user) do + token = Demo.Accounts.generate_session_token(user) + + conn + |> Phoenix.ConnTest.init_test_session(%{}) + |> Plug.Conn.put_session(:user_token, token) + end end From 3558bac96bf0f5eb0a10c0b3b35b86a220feba69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 11:22:00 +0100 Subject: [PATCH 23/76] Session controller --- lib/demo_web/controllers/user_auth.ex | 8 ++- test/demo_web/controllers/user_auth_test.exs | 18 +++++++ .../user_session_controller_test.exs | 51 +++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index 1698c43..ecafd39 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -123,11 +123,17 @@ defmodule DemoWeb.UserAuth do else conn |> put_flash(:error, "You must login to access this page.") - |> put_session(:user_return_to, conn.request_path) + |> maybe_store_return_to() |> redirect(to: Routes.user_session_path(conn, :new)) |> halt() end end + defp maybe_store_return_to(%{method: "GET", request_path: request_path} = conn) do + put_session(conn, :user_return_to, request_path) + end + + defp maybe_store_return_to(conn), do: conn + defp signed_in_path(_conn), do: "/" end diff --git a/test/demo_web/controllers/user_auth_test.exs b/test/demo_web/controllers/user_auth_test.exs index b43746f..da42420 100644 --- a/test/demo_web/controllers/user_auth_test.exs +++ b/test/demo_web/controllers/user_auth_test.exs @@ -121,6 +121,24 @@ defmodule DemoWeb.UserAuthTest do assert get_flash(conn, :error) == "You must login to access this page." end + test "stores the path to redirect to on GET", %{conn: conn} do + halted_conn = + %{conn | request_path: "/foo?bar"} + |> fetch_flash() + |> UserAuth.require_authenticated_user([]) + + assert halted_conn.halted + assert get_session(halted_conn, :user_return_to) == "/foo?bar" + + halted_conn = + %{conn | request_path: "/foo?bar", method: "POST"} + |> fetch_flash() + |> UserAuth.require_authenticated_user([]) + + assert halted_conn.halted + refute get_session(halted_conn, :user_return_to) + end + test "does not redirect if user is not authenticated", %{conn: conn, user: user} do conn = conn |> assign(:current_user, user) |> UserAuth.require_authenticated_user([]) refute conn.halted diff --git a/test/demo_web/controllers/user_session_controller_test.exs b/test/demo_web/controllers/user_session_controller_test.exs index c5f0d9e..1b78b36 100644 --- a/test/demo_web/controllers/user_session_controller_test.exs +++ b/test/demo_web/controllers/user_session_controller_test.exs @@ -18,4 +18,55 @@ defmodule DemoWeb.UserSessionControllerTest do assert redirected_to(conn) == "/" end end + + describe "POST /users/login" do + test "logs the user in", %{conn: conn, user: user} do + conn = + post(conn, Routes.user_session_path(conn, :new), %{ + "user" => %{"email" => user.email, "password" => valid_user_password()} + }) + + assert get_session(conn, :user_token) + assert redirected_to(conn) =~ "/" + end + + test "logs the user in with remember me", %{conn: conn, user: user} do + conn = + post(conn, Routes.user_session_path(conn, :new), %{ + "user" => %{ + "email" => user.email, + "password" => valid_user_password(), + "remember_me" => "true" + } + }) + + assert conn.resp_cookies["user_remember_me"] + assert redirected_to(conn) =~ "/" + end + + test "emits error message with invalid credentials", %{conn: conn, user: user} do + conn = + post(conn, Routes.user_session_path(conn, :new), %{ + "user" => %{"email" => user.email, "password" => "invalid_password"} + }) + + response = html_response(conn, 200) + assert response =~ "

    Login

    " + assert response =~ "Invalid e-mail or password" + end + end + + describe "DELETE /users/logout" do + test "redirects if not logged in", %{conn: conn} do + conn = delete(conn, Routes.user_session_path(conn, :delete)) + assert redirected_to(conn) == "/users/login" + end + + test "logs the user out", %{conn: conn, user: user} do + conn = conn |> login_user(user) |> delete(Routes.user_session_path(conn, :delete)) + assert redirected_to(conn) == "/" + refute get_session(conn, :user_token) + assert get_flash(conn, :info) =~ "Logged out successfully" + end + end end From bdef99d2ea752e3316a19264b027604ec6f849a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 11:48:04 +0100 Subject: [PATCH 24/76] Registration controller --- lib/demo/accounts/user_notifier.ex | 10 ++-- .../user_registration_controller_test.exs | 54 +++++++++++++++++++ .../user_session_controller_test.exs | 18 +++++-- test/support/fixtures/accounts_fixtures.ex | 2 +- 4 files changed, 75 insertions(+), 9 deletions(-) create mode 100644 test/demo_web/controllers/user_registration_controller_test.exs diff --git a/lib/demo/accounts/user_notifier.ex b/lib/demo/accounts/user_notifier.ex index 2a20f06..00b4945 100644 --- a/lib/demo/accounts/user_notifier.ex +++ b/lib/demo/accounts/user_notifier.ex @@ -1,16 +1,18 @@ defmodule Demo.Accounts.UserNotifier do - # For simplicity, this module simply prints messages to the terminal. + # For simplicity, this module simply logs messages to the terminal. # You should replace it by a proper e-mail or notification tool, such as: # # * Swoosh - https://github.com/swoosh/swoosh # * Bamboo - https://github.com/thoughtbot/bamboo # + require Logger + @doc """ Deliver instructions to confirm account. """ def deliver_confirmation_instructions(user, url) do - IO.puts(""" + Logger.warn(""" ============================== @@ -30,7 +32,7 @@ defmodule Demo.Accounts.UserNotifier do Deliver instructions to reset password account. """ def deliver_reset_password_instructions(user, url) do - IO.puts(""" + Logger.warn(""" ============================== @@ -50,7 +52,7 @@ defmodule Demo.Accounts.UserNotifier do Deliver instructions to update your e-mail. """ def deliver_update_email_instructions(user, url) do - IO.puts(""" + Logger.warn(""" ============================== diff --git a/test/demo_web/controllers/user_registration_controller_test.exs b/test/demo_web/controllers/user_registration_controller_test.exs new file mode 100644 index 0000000..cbcaa1e --- /dev/null +++ b/test/demo_web/controllers/user_registration_controller_test.exs @@ -0,0 +1,54 @@ +defmodule DemoWeb.UserRegistrationControllerTest do + use DemoWeb.ConnCase + + import Demo.AccountsFixtures + + describe "GET /users/register" do + test "renders registraation page", %{conn: conn} do + conn = get(conn, Routes.user_registration_path(conn, :new)) + response = html_response(conn, 200) + assert response =~ "

    Register

    " + assert response =~ "Login" + assert response =~ "Register" + end + + test "redirects if already logged in", %{conn: conn} do + conn = conn |> login_user(user_fixture()) |> get(Routes.user_registration_path(conn, :new)) + assert redirected_to(conn) == "/" + end + end + + describe "POST /users/register" do + @tag :capture_log + test "creates and account logs the user in", %{conn: conn} do + email = unique_user_email() + + conn = + post(conn, Routes.user_registration_path(conn, :create), %{ + "user" => %{"email" => email, "password" => valid_user_password()} + }) + + assert get_session(conn, :user_token) + assert redirected_to(conn) =~ "/" + + # Now do a logged in request and assert on the menu + conn = get(conn, "/") + response = html_response(conn, 200) + assert response =~ email + assert response =~ "Settings" + assert response =~ "Logout" + end + + test "render errors for invalid data", %{conn: conn} do + conn = + post(conn, Routes.user_registration_path(conn, :create), %{ + "user" => %{"email" => "with spaces", "password" => "too short"} + }) + + response = html_response(conn, 200) + assert response =~ "

    Register

    " + assert response =~ "must have the @ sign and no spaces" + assert response =~ "should be at least 12 character" + end + end +end diff --git a/test/demo_web/controllers/user_session_controller_test.exs b/test/demo_web/controllers/user_session_controller_test.exs index 1b78b36..3f97097 100644 --- a/test/demo_web/controllers/user_session_controller_test.exs +++ b/test/demo_web/controllers/user_session_controller_test.exs @@ -10,7 +10,10 @@ defmodule DemoWeb.UserSessionControllerTest do describe "GET /users/login" do test "renders login page", %{conn: conn} do conn = get(conn, Routes.user_session_path(conn, :new)) - assert html_response(conn, 200) =~ "

    Login

    " + response = html_response(conn, 200) + assert response =~ "

    Login

    " + assert response =~ "Login" + assert response =~ "Register" end test "redirects if already logged in", %{conn: conn, user: user} do @@ -22,17 +25,24 @@ defmodule DemoWeb.UserSessionControllerTest do describe "POST /users/login" do test "logs the user in", %{conn: conn, user: user} do conn = - post(conn, Routes.user_session_path(conn, :new), %{ + post(conn, Routes.user_session_path(conn, :create), %{ "user" => %{"email" => user.email, "password" => valid_user_password()} }) assert get_session(conn, :user_token) assert redirected_to(conn) =~ "/" + + # Now do a logged in request and assert on the menu + conn = get(conn, "/") + response = html_response(conn, 200) + assert response =~ user.email + assert response =~ "Settings" + assert response =~ "Logout" end test "logs the user in with remember me", %{conn: conn, user: user} do conn = - post(conn, Routes.user_session_path(conn, :new), %{ + post(conn, Routes.user_session_path(conn, :create), %{ "user" => %{ "email" => user.email, "password" => valid_user_password(), @@ -46,7 +56,7 @@ defmodule DemoWeb.UserSessionControllerTest do test "emits error message with invalid credentials", %{conn: conn, user: user} do conn = - post(conn, Routes.user_session_path(conn, :new), %{ + post(conn, Routes.user_session_path(conn, :create), %{ "user" => %{"email" => user.email, "password" => "invalid_password"} }) diff --git a/test/support/fixtures/accounts_fixtures.ex b/test/support/fixtures/accounts_fixtures.ex index 7a309a6..760ecfe 100644 --- a/test/support/fixtures/accounts_fixtures.ex +++ b/test/support/fixtures/accounts_fixtures.ex @@ -16,7 +16,7 @@ defmodule Demo.AccountsFixtures do def capture_user_token(fun) do captured = - ExUnit.CaptureIO.capture_io(fn -> + ExUnit.CaptureLog.capture_log(fn -> fun.(&"[TOKEN]#{&1}[TOKEN]") end) From e61a7aa86334db4d887d70cd35f8b3a6f2fb2d85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 12:43:45 +0100 Subject: [PATCH 25/76] Settings controller --- .../user_registration_controller_test.exs | 4 +- .../user_settings_controller_test.exs | 121 ++++++++++++++++++ test/support/conn_case.ex | 18 +++ 3 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 test/demo_web/controllers/user_settings_controller_test.exs diff --git a/test/demo_web/controllers/user_registration_controller_test.exs b/test/demo_web/controllers/user_registration_controller_test.exs index cbcaa1e..77205c1 100644 --- a/test/demo_web/controllers/user_registration_controller_test.exs +++ b/test/demo_web/controllers/user_registration_controller_test.exs @@ -4,7 +4,7 @@ defmodule DemoWeb.UserRegistrationControllerTest do import Demo.AccountsFixtures describe "GET /users/register" do - test "renders registraation page", %{conn: conn} do + test "renders registration page", %{conn: conn} do conn = get(conn, Routes.user_registration_path(conn, :new)) response = html_response(conn, 200) assert response =~ "

    Register

    " @@ -20,7 +20,7 @@ defmodule DemoWeb.UserRegistrationControllerTest do describe "POST /users/register" do @tag :capture_log - test "creates and account logs the user in", %{conn: conn} do + test "creates account and logs the user in", %{conn: conn} do email = unique_user_email() conn = diff --git a/test/demo_web/controllers/user_settings_controller_test.exs b/test/demo_web/controllers/user_settings_controller_test.exs new file mode 100644 index 0000000..71ec0ab --- /dev/null +++ b/test/demo_web/controllers/user_settings_controller_test.exs @@ -0,0 +1,121 @@ +defmodule DemoWeb.UserSettingsControllerTest do + use DemoWeb.ConnCase + + alias Demo.Accounts + import Demo.AccountsFixtures + + setup :register_and_login_user + + describe "GET /users/settings" do + test "renders settings page", %{conn: conn} do + conn = get(conn, Routes.user_settings_path(conn, :edit)) + response = html_response(conn, 200) + assert response =~ "

    Settings

    " + end + + test "redirects if user is not logged in" do + conn = build_conn() + conn = get(conn, Routes.user_settings_path(conn, :edit)) + assert redirected_to(conn) == "/users/login" + end + end + + describe "PUT /users/settings/update_password" do + test "updates the user password and resets tokens", %{conn: conn, user: user} do + new_password_conn = + put(conn, Routes.user_settings_path(conn, :update_password), %{ + "current_password" => valid_user_password(), + "user" => %{ + "password" => "new valid password", + "password_confirmation" => "new valid password" + } + }) + + assert redirected_to(new_password_conn) == "/users/settings" + assert get_session(new_password_conn, :user_token) != get_session(conn, :user_token) + assert get_flash(new_password_conn, :info) =~ "Password updated successfully" + assert Accounts.get_user_by_email_and_password(user.email, "new valid password") + end + + test "does not update password on invalid data", %{conn: conn} do + old_password_conn = + put(conn, Routes.user_settings_path(conn, :update_password), %{ + "current_password" => "invalid", + "user" => %{ + "password" => "too short", + "password_confirmation" => "does not match" + } + }) + + response = html_response(old_password_conn, 200) + assert response =~ "

    Settings

    " + assert response =~ "should be at least 12 character(s)" + assert response =~ "does not match confirmation" + assert response =~ "is not valid" + + assert get_session(old_password_conn, :user_token) == get_session(conn, :user_token) + end + end + + describe "PUT /users/settings/update_email" do + @tag :capture_log + test "updates the user email", %{conn: conn, user: user} do + conn = + put(conn, Routes.user_settings_path(conn, :update_email), %{ + "current_password" => valid_user_password(), + "user" => %{"email" => unique_user_email()} + }) + + assert redirected_to(conn) == "/users/settings" + assert get_flash(conn, :info) =~ "A link to confirm your e-mail" + assert Accounts.get_user_by_email(user.email) + end + + test "does not update email on invalid data", %{conn: conn} do + conn = + put(conn, Routes.user_settings_path(conn, :update_email), %{ + "current_password" => "invalid", + "user" => %{"email" => "with spaces"} + }) + + response = html_response(conn, 200) + assert response =~ "

    Settings

    " + assert response =~ "must have the @ sign and no spaces" + assert response =~ "is not valid" + end + end + + describe "GET /users/settings/confirm_email" do + setup %{user: user} do + email = unique_user_email() + + token = + capture_user_token(fn url -> + Accounts.deliver_update_email_instructions(%{user | email: email}, user.email, url) + end) + + %{token: token, email: email} + end + + test "updates the user email", %{conn: conn, user: user, token: token, email: email} do + conn = get(conn, Routes.user_settings_path(conn, :confirm_email, token)) + assert redirected_to(conn) == "/users/settings" + assert get_flash(conn, :info) =~ "E-mail changed successfully" + refute Accounts.get_user_by_email(user.email) + assert Accounts.get_user_by_email(email) + end + + test "does not update email with invalid token", %{conn: conn, user: user} do + conn = get(conn, Routes.user_settings_path(conn, :confirm_email, "oops")) + assert redirected_to(conn) == "/users/settings" + assert get_flash(conn, :error) =~ "Email change token is invalid or it has expired" + assert Accounts.get_user_by_email(user.email) + end + + test "redirects if user is not logged in", %{token: token} do + conn = build_conn() + conn = get(conn, Routes.user_settings_path(conn, :confirm_email, token)) + assert redirected_to(conn) == "/users/login" + end + end +end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 39e0835..c39ea94 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -41,6 +41,24 @@ defmodule DemoWeb.ConnCase do {:ok, conn: Phoenix.ConnTest.build_conn()} end + @doc """ + Setup helper that registers and logs in users. + + setup :register_and_login_user + + It stores an updated connection and a register user in the + test context. + """ + def register_and_login_user(%{conn: conn}) do + user = Demo.AccountsFixtures.user_fixture() + %{conn: login_user(conn, user), user: user} + end + + @doc """ + Logs the given `user` into the `conn`. + + It returns an updated `conn`. + """ def login_user(conn, user) do token = Demo.Accounts.generate_session_token(user) From fc2c7350626f10550c87bb7aa02fdc439995f239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 13:49:49 +0100 Subject: [PATCH 26/76] Confirmation controller tests --- .../user_confirmation_controller_test.exs | 84 +++++++++++++++++++ .../user_settings_controller_test.exs | 8 +- 2 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 test/demo_web/controllers/user_confirmation_controller_test.exs diff --git a/test/demo_web/controllers/user_confirmation_controller_test.exs b/test/demo_web/controllers/user_confirmation_controller_test.exs new file mode 100644 index 0000000..4fbf051 --- /dev/null +++ b/test/demo_web/controllers/user_confirmation_controller_test.exs @@ -0,0 +1,84 @@ +defmodule DemoWeb.UserconfirmationControllerTest do + use DemoWeb.ConnCase + + alias Demo.Accounts + alias Demo.Repo + import Demo.AccountsFixtures + + setup do + %{user: user_fixture()} + end + + describe "GET /users/confirm" do + test "renders the confirmation page", %{conn: conn} do + conn = get(conn, Routes.user_confirmation_path(conn, :new)) + response = html_response(conn, 200) + assert response =~ "

    Resend confirmation instructions

    " + end + end + + describe "POST /users/confirm" do + @tag :capture_log + test "sends a new confirmation token", %{conn: conn, user: user} do + conn = + post(conn, Routes.user_confirmation_path(conn, :create), %{ + "user" => %{"email" => user.email} + }) + + assert redirected_to(conn) == "/" + assert get_flash(conn, :info) =~ "If your e-mail is in our system" + assert Repo.get_by!(Accounts.UserToken, user_id: user.id).context == "confirm" + end + + test "does not send confirmation token if account is confirmed", %{conn: conn, user: user} do + Repo.update!(Accounts.User.confirm_changeset(user)) + + conn = + post(conn, Routes.user_confirmation_path(conn, :create), %{ + "user" => %{"email" => user.email} + }) + + assert redirected_to(conn) == "/" + assert get_flash(conn, :info) =~ "If your e-mail is in our system" + refute Repo.get_by(Accounts.UserToken, user_id: user.id) + end + + test "does not send confirmation token if email is invalid", %{conn: conn} do + conn = + post(conn, Routes.user_confirmation_path(conn, :create), %{ + "user" => %{"email" => "unknown@example.com"} + }) + + assert redirected_to(conn) == "/" + assert get_flash(conn, :info) =~ "If your e-mail is in our system" + assert Repo.all(Accounts.UserToken) == [] + end + end + + describe "GET /users/confirm/:token" do + test "confirms the given token once", %{conn: conn, user: user} do + token = + capture_user_token(fn url -> + Accounts.deliver_user_confirmation_instructions(user, url) + end) + + conn = get(conn, Routes.user_confirmation_path(conn, :confirm, token)) + assert redirected_to(conn) == "/" + assert get_flash(conn, :info) =~ "Account confirmed successfully" + assert Accounts.get_user!(user.id).confirmed_at + refute get_session(conn, :user_token) + assert Repo.all(Accounts.UserToken) == [] + + conn = get(conn, Routes.user_confirmation_path(conn, :confirm, token)) + assert redirected_to(conn) == "/" + assert get_flash(conn, :error) =~ "Confirmation token is invalid or it has expired" + end + + test "does not confirm email with invalid token", %{conn: conn, user: user} do + conn = get(conn, Routes.user_confirmation_path(conn, :confirm, "oops")) + assert redirected_to(conn) == "/" + assert get_flash(conn, :error) =~ "Confirmation token is invalid or it has expired" + refute Accounts.get_user!(user.id).confirmed_at + end + end +end diff --git a/test/demo_web/controllers/user_settings_controller_test.exs b/test/demo_web/controllers/user_settings_controller_test.exs index 71ec0ab..ca00072 100644 --- a/test/demo_web/controllers/user_settings_controller_test.exs +++ b/test/demo_web/controllers/user_settings_controller_test.exs @@ -85,7 +85,7 @@ defmodule DemoWeb.UserSettingsControllerTest do end end - describe "GET /users/settings/confirm_email" do + describe "GET /users/settings/confirm_email/:token" do setup %{user: user} do email = unique_user_email() @@ -97,12 +97,16 @@ defmodule DemoWeb.UserSettingsControllerTest do %{token: token, email: email} end - test "updates the user email", %{conn: conn, user: user, token: token, email: email} do + test "updates the user email once", %{conn: conn, user: user, token: token, email: email} do conn = get(conn, Routes.user_settings_path(conn, :confirm_email, token)) assert redirected_to(conn) == "/users/settings" assert get_flash(conn, :info) =~ "E-mail changed successfully" refute Accounts.get_user_by_email(user.email) assert Accounts.get_user_by_email(email) + + conn = get(conn, Routes.user_settings_path(conn, :confirm_email, token)) + assert redirected_to(conn) == "/users/settings" + assert get_flash(conn, :error) =~ "Email change token is invalid or it has expired" end test "does not update email with invalid token", %{conn: conn, user: user} do From 76ff10ab78fe282461d5101ae4853e22e3d4b6c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 14:50:29 +0100 Subject: [PATCH 27/76] Reset tests --- lib/demo/accounts/user_notifier.ex | 5 +- .../user_reset_password_controller.ex | 1 + .../user_reset_password_controller_test.exs | 113 ++++++++++++++++++ 3 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 test/demo_web/controllers/user_reset_password_controller_test.exs diff --git a/lib/demo/accounts/user_notifier.ex b/lib/demo/accounts/user_notifier.ex index 00b4945..1638e21 100644 --- a/lib/demo/accounts/user_notifier.ex +++ b/lib/demo/accounts/user_notifier.ex @@ -2,10 +2,9 @@ defmodule Demo.Accounts.UserNotifier do # For simplicity, this module simply logs messages to the terminal. # You should replace it by a proper e-mail or notification tool, such as: # - # * Swoosh - https://github.com/swoosh/swoosh - # * Bamboo - https://github.com/thoughtbot/bamboo + # * Swoosh - https://hexdocs.pm/swoosh + # * Bamboo - https://hexdocs.pm/bamboo # - require Logger @doc """ diff --git a/lib/demo_web/controllers/user_reset_password_controller.ex b/lib/demo_web/controllers/user_reset_password_controller.ex index 087113b..c1f9f92 100644 --- a/lib/demo_web/controllers/user_reset_password_controller.ex +++ b/lib/demo_web/controllers/user_reset_password_controller.ex @@ -53,6 +53,7 @@ defmodule DemoWeb.UserResetPasswordController do conn |> put_flash(:error, "Reset password token is invalid or it has expired.") |> redirect(to: "/") + |> halt() end end end diff --git a/test/demo_web/controllers/user_reset_password_controller_test.exs b/test/demo_web/controllers/user_reset_password_controller_test.exs new file mode 100644 index 0000000..2ff6eb3 --- /dev/null +++ b/test/demo_web/controllers/user_reset_password_controller_test.exs @@ -0,0 +1,113 @@ +defmodule DemoWeb.UserResetPasswordControllerTest do + use DemoWeb.ConnCase + + alias Demo.Accounts + alias Demo.Repo + import Demo.AccountsFixtures + + setup do + %{user: user_fixture()} + end + + describe "GET /users/reset_password" do + test "renders the reset password page", %{conn: conn} do + conn = get(conn, Routes.user_reset_password_path(conn, :new)) + response = html_response(conn, 200) + assert response =~ "

    Forgot your password?

    " + end + end + + describe "POST /users/reset_password" do + @tag :capture_log + test "sends a new reset password token", %{conn: conn, user: user} do + conn = + post(conn, Routes.user_reset_password_path(conn, :create), %{ + "user" => %{"email" => user.email} + }) + + assert redirected_to(conn) == "/" + assert get_flash(conn, :info) =~ "If your e-mail is in our system" + assert Repo.get_by!(Accounts.UserToken, user_id: user.id).context == "reset_password" + end + + test "does not send reset password token if email is invalid", %{conn: conn} do + conn = + post(conn, Routes.user_reset_password_path(conn, :create), %{ + "user" => %{"email" => "unknown@example.com"} + }) + + assert redirected_to(conn) == "/" + assert get_flash(conn, :info) =~ "If your e-mail is in our system" + assert Repo.all(Accounts.UserToken) == [] + end + end + + describe "GET /users/reset_password/:token" do + setup %{user: user} do + token = + capture_user_token(fn url -> + Accounts.deliver_user_reset_password_instructions(user, url) + end) + + %{token: token} + end + + test "renders reset password", %{conn: conn, token: token} do + conn = get(conn, Routes.user_reset_password_path(conn, :edit, token)) + assert html_response(conn, 200) =~ "

    Reset password

    " + end + + test "does not render reset password with invalid token", %{conn: conn} do + conn = get(conn, Routes.user_reset_password_path(conn, :edit, "oops")) + assert redirected_to(conn) == "/" + assert get_flash(conn, :error) =~ "Reset password token is invalid or it has expired" + end + end + + describe "PUT /users/reset_password/:token" do + setup %{user: user} do + token = + capture_user_token(fn url -> + Accounts.deliver_user_reset_password_instructions(user, url) + end) + + %{token: token} + end + + test "resets password once", %{conn: conn, user: user, token: token} do + conn = + put(conn, Routes.user_reset_password_path(conn, :update, token), %{ + "user" => %{ + "password" => "new valid password", + "password_confirmation" => "new valid password" + } + }) + + assert redirected_to(conn) == "/users/login" + refute get_session(conn, :user_token) + assert get_flash(conn, :info) =~ "Password reset successfully" + assert Accounts.get_user_by_email_and_password(user.email, "new valid password") + end + + test "does not reset password on invalid data", %{conn: conn, token: token} do + conn = + put(conn, Routes.user_reset_password_path(conn, :update, token), %{ + "user" => %{ + "password" => "too short", + "password_confirmation" => "does not match" + } + }) + + response = html_response(conn, 200) + assert response =~ "

    Reset password

    " + assert response =~ "should be at least 12 character(s)" + assert response =~ "does not match confirmation" + end + + test "does not reset password with invalid token", %{conn: conn} do + conn = put(conn, Routes.user_reset_password_path(conn, :update, "oops")) + assert redirected_to(conn) == "/" + assert get_flash(conn, :error) =~ "Reset password token is invalid or it has expired" + end + end +end From 44a6bacf9fa70ec0e03a2fef2585979216efeaf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 16:48:09 +0100 Subject: [PATCH 28/76] Apply suggestions from code review Co-Authored-By: Aaron Renner --- test/support/conn_case.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index c39ea94..09d8764 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -46,10 +46,10 @@ defmodule DemoWeb.ConnCase do setup :register_and_login_user - It stores an updated connection and a register user in the + It stores an updated connection and a registered user in the test context. """ - def register_and_login_user(%{conn: conn}) do + def register_and_login_user(%{conn: conn}) do user = Demo.AccountsFixtures.user_fixture() %{conn: login_user(conn, user), user: user} end From 7c07c028707b5932ed5e0f072cdecef5c82c7478 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 17:12:46 +0100 Subject: [PATCH 29/76] Add a license --- README.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 12767e6..70b92a6 100644 --- a/README.md +++ b/README.md @@ -23,4 +23,18 @@ For changing the e-mail, the user will put the new e-mail in a field. This will mix phx.gen.auth Account User users ...extrafields... -The authentication mechanism should be an option. Default to bcrypt for Unix systems, pdkdf2 for Windows systems. The line to config/test.exs must always be added. \ No newline at end of file +The authentication mechanism should be an option. Default to bcrypt for Unix systems, pdkdf2 for Windows systems. The line to config/test.exs must always be added. + +## License + +Copyright 2020 Dashbit + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0) + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. From 085dd9292975fd783744ccaa41b37ef5c94587a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 18:00:45 +0100 Subject: [PATCH 30/76] Use citext for PG --- ...ables.exs => 20200316103722_create_user_auth_tables.exs} | 6 ++++-- test/demo/accounts_test.exs | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) rename priv/repo/migrations/{20200316103722_create_auth_tables.exs => 20200316103722_create_user_auth_tables.exs} (78%) diff --git a/priv/repo/migrations/20200316103722_create_auth_tables.exs b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs similarity index 78% rename from priv/repo/migrations/20200316103722_create_auth_tables.exs rename to priv/repo/migrations/20200316103722_create_user_auth_tables.exs index 76e2b87..ffcd642 100644 --- a/priv/repo/migrations/20200316103722_create_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs @@ -1,9 +1,11 @@ -defmodule Demo.Repo.Migrations.CreateAuthTables do +defmodule Demo.Repo.Migrations.CreateUserAuthTables do use Ecto.Migration def change do + execute "CREATE EXTENSION IF NOT EXISTS citext", "" + create table(:users) do - add :email, :string, null: false + add :email, :citext, null: false add :encrypted_password, :string, null: false add :confirmed_at, :naive_datetime timestamps() diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index bcd5834..bf995f1 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -77,6 +77,10 @@ defmodule Demo.AccountsTest do %{email: email} = user_fixture() {:error, changeset} = Accounts.register_user(%{email: email}) assert "has already been taken" in errors_on(changeset).email + + # Now try with the upcase e-mail too + {:error, changeset} = Accounts.register_user(%{email: String.upcase(email)}) + assert "has already been taken" in errors_on(changeset).email end test "registers users with an encrypted password" do From 0e3efd92bb8e9708bd48d7d57256d7a866ba0940 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 19:17:17 +0100 Subject: [PATCH 31/76] Update lib/demo_web/router.ex Co-Authored-By: Aaron Renner --- lib/demo_web/router.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/demo_web/router.ex b/lib/demo_web/router.ex index f859c7a..4ec47b1 100644 --- a/lib/demo_web/router.ex +++ b/lib/demo_web/router.ex @@ -27,7 +27,7 @@ defmodule DemoWeb.Router do # pipe_through :api # end - ## New routes + ## Authentication routes scope "/", DemoWeb do pipe_through [:browser, :redirect_if_user_is_authenticated] From fcf4a8be018234e496c2256cad0d36c0bf185b63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 20:54:37 +0100 Subject: [PATCH 32/76] More validations as suggestions --- lib/demo/accounts/user.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index cae33f8..098f1ff 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -39,6 +39,9 @@ defmodule Demo.Accounts.User do changeset |> validate_required([:password]) |> validate_length(:password, min: 12, max: 80) + # |> validate_format(:password, ~r/[a-z]/, message: "at least one lower case character") + # |> validate_format(:password, ~r/[A-Z]/, message: "at least one upper case character") + # |> validate_format(:password, ~r/[!?@#$%^&*_0-9]/, message: "at least one digit or punctuation character") |> maybe_encrypt_password() end From 911346656ef0419727a4075066ecd0e9a198e7df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 21:13:35 +0100 Subject: [PATCH 33/76] Update test/demo_web/controllers/user_confirmation_controller_test.exs Co-Authored-By: Aaron Renner --- test/demo_web/controllers/user_confirmation_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/demo_web/controllers/user_confirmation_controller_test.exs b/test/demo_web/controllers/user_confirmation_controller_test.exs index 4fbf051..44d8fbe 100644 --- a/test/demo_web/controllers/user_confirmation_controller_test.exs +++ b/test/demo_web/controllers/user_confirmation_controller_test.exs @@ -1,4 +1,4 @@ -defmodule DemoWeb.UserconfirmationControllerTest do +defmodule DemoWeb.UserConfirmationControllerTest do use DemoWeb.ConnCase alias Demo.Accounts From 0712d9c2137b56e0b54d046807918b59335b5c4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 26 Mar 2020 23:58:05 +0100 Subject: [PATCH 34/76] Rename file properly --- .../{user_confirmation.ex => user_confirmation_controller.ex} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename lib/demo_web/controllers/{user_confirmation.ex => user_confirmation_controller.ex} (100%) diff --git a/lib/demo_web/controllers/user_confirmation.ex b/lib/demo_web/controllers/user_confirmation_controller.ex similarity index 100% rename from lib/demo_web/controllers/user_confirmation.ex rename to lib/demo_web/controllers/user_confirmation_controller.ex From 9c2287014c0d70ab4a4c026ab9ef9c75cec9ecb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 27 Mar 2020 21:41:42 +0100 Subject: [PATCH 35/76] Apply suggestions from code review Co-Authored-By: Aaron Renner --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 70b92a6..245c0f2 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ ## Spec -We will two database tables: "users" and "users_tokens": +We will have two database tables: "users" and "users_tokens": * "users" will have the "encrypted_password", "email" and "confirmed_at" fields plus timestamps * "users_tokens" will have "token", "context", "sent_to", "inserted_at" fields From 19470c802ada5dd3797289a9f127282be51fb70b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 27 Mar 2020 21:58:30 +0100 Subject: [PATCH 36/76] Update lib/demo_web/templates/layout/_user_menu.html.eex --- lib/demo_web/templates/layout/_user_menu.html.eex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/demo_web/templates/layout/_user_menu.html.eex b/lib/demo_web/templates/layout/_user_menu.html.eex index 76ec857..a31996a 100644 --- a/lib/demo_web/templates/layout/_user_menu.html.eex +++ b/lib/demo_web/templates/layout/_user_menu.html.eex @@ -7,4 +7,4 @@
  • <%= link "Register", to: Routes.user_registration_path(@conn, :new) %>
  • <%= link "Login", to: Routes.user_session_path(@conn, :new) %>
    • <% end %> -
\ No newline at end of file + From 4796235d22870f36c3dd1e674a09cc4725eeaf76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 30 Mar 2020 16:36:58 +0200 Subject: [PATCH 37/76] Tokens must be unique --- priv/repo/migrations/20200316103722_create_user_auth_tables.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs index ffcd642..0bf931a 100644 --- a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs @@ -21,6 +21,6 @@ defmodule Demo.Repo.Migrations.CreateUserAuthTables do add :inserted_at, :naive_datetime end - create index(:user_tokens, [:user_id, :token]) + create unique_index(:user_tokens, [:token]) end end From f801205c70be544f78cb4c8f5026e92627e6122c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 30 Mar 2020 16:43:48 +0200 Subject: [PATCH 38/76] Add tests, improve index --- .../20200316103722_create_user_auth_tables.exs | 2 +- test/demo/accounts_test.exs | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs index 0bf931a..f7b1ba3 100644 --- a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs @@ -21,6 +21,6 @@ defmodule Demo.Repo.Migrations.CreateUserAuthTables do add :inserted_at, :naive_datetime end - create unique_index(:user_tokens, [:token]) + create unique_index(:user_tokens, [:context, :token]) end end diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index bf995f1..4348ff7 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -290,6 +290,15 @@ defmodule Demo.AccountsTest do token = Accounts.generate_session_token(user) assert user_token = Repo.get_by(UserToken, token: token) assert user_token.context == "session" + + # Creating the same token for another user fail + assert_raise Ecto.ConstraintError, fn -> + Repo.insert!(%UserToken{ + token: user_token.token, + user_id: user_fixture().id, + context: "session" + }) + end end end From 2a7616ac341d4a5f58ed760078bf2f3057327d55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 31 Mar 2020 21:43:57 +0200 Subject: [PATCH 39/76] Return a mail structure from notifier --- lib/demo/accounts.ex | 9 +++----- lib/demo/accounts/user_notifier.ex | 12 +++++++---- .../user_registration_controller.ex | 2 +- test/demo/accounts_test.exs | 21 ++++++++----------- .../user_confirmation_controller_test.exs | 2 +- .../user_reset_password_controller_test.exs | 4 ++-- .../user_settings_controller_test.exs | 2 +- test/support/fixtures/accounts_fixtures.ex | 10 +++------ 8 files changed, 28 insertions(+), 34 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 6e9adf5..1d41f38 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -158,7 +158,7 @@ defmodule Demo.Accounts do ## Examples iex> deliver_update_email_instructions(user, current_email, &Routes.user_update_email_url(conn, :edit)) - :ok + {:ok, %{to: ..., body: ...}} """ def deliver_update_email_instructions(%User{} = user, current_email, update_email_url_fun) @@ -168,7 +168,6 @@ defmodule Demo.Accounts do Repo.insert!(user_token) UserNotifier.deliver_update_email_instructions(user, update_email_url_fun.(encoded_token)) - :ok end @doc """ @@ -247,7 +246,7 @@ defmodule Demo.Accounts do ## Examples iex> deliver_user_confirmation_instructions(user, &Routes.user_confirmation_url(conn, :confirm)) - :ok + {:ok, %{to: ..., body: ...}} iex> deliver_user_confirmation_instructions(confirmed_user, &Routes.user_confirmation_url(conn, :confirm)) {:error, :already_confirmed} @@ -261,7 +260,6 @@ defmodule Demo.Accounts do {encoded_token, user_token} = UserToken.build_user_email_token(user, "confirm") Repo.insert!(user_token) UserNotifier.deliver_confirmation_instructions(user, confirmation_url_fun.(encoded_token)) - :ok end end @@ -295,7 +293,7 @@ defmodule Demo.Accounts do ## Examples iex> deliver_user_reset_password_instructions(user, &Routes.user_reset_password_url(conn, :edit)) - :ok + {:ok, %{to: ..., body: ...}} """ def deliver_user_reset_password_instructions(%User{} = user, reset_password_url_fun) @@ -303,7 +301,6 @@ defmodule Demo.Accounts do {encoded_token, user_token} = UserToken.build_user_email_token(user, "reset_password") Repo.insert!(user_token) UserNotifier.deliver_reset_password_instructions(user, reset_password_url_fun.(encoded_token)) - :ok end @doc """ diff --git a/lib/demo/accounts/user_notifier.ex b/lib/demo/accounts/user_notifier.ex index 1638e21..a2310c7 100644 --- a/lib/demo/accounts/user_notifier.ex +++ b/lib/demo/accounts/user_notifier.ex @@ -5,13 +5,17 @@ defmodule Demo.Accounts.UserNotifier do # * Swoosh - https://hexdocs.pm/swoosh # * Bamboo - https://hexdocs.pm/bamboo # - require Logger + defp deliver(to, body) do + require Logger + Logger.debug(body) + {:ok, %{to: to, body: body}} + end @doc """ Deliver instructions to confirm account. """ def deliver_confirmation_instructions(user, url) do - Logger.warn(""" + deliver(user.email, """ ============================== @@ -31,7 +35,7 @@ defmodule Demo.Accounts.UserNotifier do Deliver instructions to reset password account. """ def deliver_reset_password_instructions(user, url) do - Logger.warn(""" + deliver(user.email, """ ============================== @@ -51,7 +55,7 @@ defmodule Demo.Accounts.UserNotifier do Deliver instructions to update your e-mail. """ def deliver_update_email_instructions(user, url) do - Logger.warn(""" + deliver(user.email, """ ============================== diff --git a/lib/demo_web/controllers/user_registration_controller.ex b/lib/demo_web/controllers/user_registration_controller.ex index 4d762a8..f52e1c6 100644 --- a/lib/demo_web/controllers/user_registration_controller.ex +++ b/lib/demo_web/controllers/user_registration_controller.ex @@ -13,7 +13,7 @@ defmodule DemoWeb.UserRegistrationController do def create(conn, %{"user" => user_params}) do case Accounts.register_user(user_params) do {:ok, user} -> - :ok = + {:ok, _} = Accounts.deliver_user_confirmation_instructions( user, &Routes.user_confirmation_url(conn, :confirm, &1) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 4348ff7..9b1936f 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -163,9 +163,8 @@ defmodule Demo.AccountsTest do test "sends token through notification", %{user: user} do token = - capture_user_token(fn url -> - assert Accounts.deliver_update_email_instructions(user, "current@example.com", url) == - :ok + extract_user_token(fn url -> + Accounts.deliver_update_email_instructions(user, "current@example.com", url) end) {:ok, token} = Base.url_decode64(token, padding: false) @@ -182,7 +181,7 @@ defmodule Demo.AccountsTest do email = unique_user_email() token = - capture_user_token(fn url -> + extract_user_token(fn url -> Accounts.deliver_update_email_instructions(%{user | email: email}, user.email, url) end) @@ -340,9 +339,8 @@ defmodule Demo.AccountsTest do test "sends token through notification", %{user: user} do token = - capture_user_token(fn url -> - assert Accounts.deliver_user_confirmation_instructions(user, url) == - :ok + extract_user_token(fn url -> + Accounts.deliver_user_confirmation_instructions(user, url) end) {:ok, token} = Base.url_decode64(token, padding: false) @@ -358,7 +356,7 @@ defmodule Demo.AccountsTest do user = user_fixture() token = - capture_user_token(fn url -> + extract_user_token(fn url -> Accounts.deliver_user_confirmation_instructions(user, url) end) @@ -394,9 +392,8 @@ defmodule Demo.AccountsTest do test "sends token through notification", %{user: user} do token = - capture_user_token(fn url -> - assert Accounts.deliver_user_reset_password_instructions(user, url) == - :ok + extract_user_token(fn url -> + Accounts.deliver_user_reset_password_instructions(user, url) end) {:ok, token} = Base.url_decode64(token, padding: false) @@ -412,7 +409,7 @@ defmodule Demo.AccountsTest do user = user_fixture() token = - capture_user_token(fn url -> + extract_user_token(fn url -> Accounts.deliver_user_reset_password_instructions(user, url) end) diff --git a/test/demo_web/controllers/user_confirmation_controller_test.exs b/test/demo_web/controllers/user_confirmation_controller_test.exs index 44d8fbe..89cb66e 100644 --- a/test/demo_web/controllers/user_confirmation_controller_test.exs +++ b/test/demo_web/controllers/user_confirmation_controller_test.exs @@ -58,7 +58,7 @@ defmodule DemoWeb.UserConfirmationControllerTest do describe "GET /users/confirm/:token" do test "confirms the given token once", %{conn: conn, user: user} do token = - capture_user_token(fn url -> + extract_user_token(fn url -> Accounts.deliver_user_confirmation_instructions(user, url) end) diff --git a/test/demo_web/controllers/user_reset_password_controller_test.exs b/test/demo_web/controllers/user_reset_password_controller_test.exs index 2ff6eb3..4699c8e 100644 --- a/test/demo_web/controllers/user_reset_password_controller_test.exs +++ b/test/demo_web/controllers/user_reset_password_controller_test.exs @@ -45,7 +45,7 @@ defmodule DemoWeb.UserResetPasswordControllerTest do describe "GET /users/reset_password/:token" do setup %{user: user} do token = - capture_user_token(fn url -> + extract_user_token(fn url -> Accounts.deliver_user_reset_password_instructions(user, url) end) @@ -67,7 +67,7 @@ defmodule DemoWeb.UserResetPasswordControllerTest do describe "PUT /users/reset_password/:token" do setup %{user: user} do token = - capture_user_token(fn url -> + extract_user_token(fn url -> Accounts.deliver_user_reset_password_instructions(user, url) end) diff --git a/test/demo_web/controllers/user_settings_controller_test.exs b/test/demo_web/controllers/user_settings_controller_test.exs index ca00072..f9e0728 100644 --- a/test/demo_web/controllers/user_settings_controller_test.exs +++ b/test/demo_web/controllers/user_settings_controller_test.exs @@ -90,7 +90,7 @@ defmodule DemoWeb.UserSettingsControllerTest do email = unique_user_email() token = - capture_user_token(fn url -> + extract_user_token(fn url -> Accounts.deliver_update_email_instructions(%{user | email: email}, user.email, url) end) diff --git a/test/support/fixtures/accounts_fixtures.ex b/test/support/fixtures/accounts_fixtures.ex index 760ecfe..c3a38fd 100644 --- a/test/support/fixtures/accounts_fixtures.ex +++ b/test/support/fixtures/accounts_fixtures.ex @@ -14,13 +14,9 @@ defmodule Demo.AccountsFixtures do user end - def capture_user_token(fun) do - captured = - ExUnit.CaptureLog.capture_log(fn -> - fun.(&"[TOKEN]#{&1}[TOKEN]") - end) - - [_, token, _] = String.split(captured, "[TOKEN]") + def extract_user_token(fun) do + {:ok, captured} = fun.(&"[TOKEN]#{&1}[TOKEN]") + [_, token, _] = String.split(captured.body, "[TOKEN]") token end end From b53f6168be79aac3cc0a843e41241197e1f2cb36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 1 Apr 2020 00:17:33 +0200 Subject: [PATCH 40/76] Move durations to module attributes and settle in days --- lib/demo/accounts/user_token.ex | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex index 384431e..fb1c30d 100644 --- a/lib/demo/accounts/user_token.ex +++ b/lib/demo/accounts/user_token.ex @@ -5,6 +5,13 @@ defmodule Demo.Accounts.UserToken do @hash_algorithm :sha256 @rand_size 32 + # It is very important to keep the reset password token expiry short, + # since someone with access to the e-mail may take over the account. + @reset_password_validity_in_days 1 + @confirm_validity_in_days 7 + @change_email_validity_in_days 7 + @session_validity_in_days 60 + schema "user_tokens" do field :token, :binary field :context, :string @@ -33,7 +40,7 @@ defmodule Demo.Accounts.UserToken do query = from token in token_and_context_query(token, "session"), join: user in assoc(token, :user), - where: token.inserted_at > ago(60, "day"), + where: token.inserted_at > ago(@session_validity_in_days, "day"), select: user {:ok, query} @@ -73,11 +80,12 @@ defmodule Demo.Accounts.UserToken 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: user in assoc(token, :user), - where: token.inserted_at > ago(1, "week") and token.sent_to == user.email, + where: token.inserted_at > ago(^days, "day") and token.sent_to == user.email, select: user {:ok, query} @@ -87,6 +95,9 @@ defmodule Demo.Accounts.UserToken do 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. @@ -99,7 +110,7 @@ defmodule Demo.Accounts.UserToken do query = from token in token_and_context_query(hashed_token, context), - where: token.inserted_at > ago(1, "week") + where: token.inserted_at > ago(@change_email_validity_in_days, "day") {:ok, query} From 60491423c0faab9c8ffdfe0a9952cd0e1bcd4a17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 1 Apr 2020 02:16:41 +0200 Subject: [PATCH 41/76] encrypted_password -> hashed_password --- README.md | 2 +- lib/demo/accounts/user.ex | 10 +++++----- .../20200316103722_create_user_auth_tables.exs | 2 +- test/demo/accounts_test.exs | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 245c0f2..77900f8 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ We will have two database tables: "users" and "users_tokens": -* "users" will have the "encrypted_password", "email" and "confirmed_at" fields plus timestamps +* "users" will have the "hashed_password", "email" and "confirmed_at" fields plus timestamps * "users_tokens" will have "token", "context", "sent_to", "inserted_at" fields On sign in, we need to renew the session and delete the CSRF token. The password should be limited to 80 characters, the email to 160. diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 098f1ff..9ab44ce 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -5,7 +5,7 @@ defmodule Demo.Accounts.User do schema "users" do field :email, :string field :password, :string, virtual: true - field :encrypted_password, :string + field :hashed_password, :string field :confirmed_at, :naive_datetime timestamps() @@ -49,7 +49,7 @@ defmodule Demo.Accounts.User do password = get_change(changeset, :password) if password && changeset.valid? do - put_change(changeset, :encrypted_password, Bcrypt.hash_pwd_salt(password)) + put_change(changeset, :hashed_password, Bcrypt.hash_pwd_salt(password)) else changeset end @@ -96,9 +96,9 @@ defmodule Demo.Accounts.User do If there is no user or the user doesn't have a password, we encrypt a blank password to avoid timing attacks. """ - def valid_password?(%Demo.Accounts.User{encrypted_password: encrypted_password}, password) - when is_binary(encrypted_password) and byte_size(password) > 0 do - Bcrypt.verify_pass(password, encrypted_password) + def valid_password?(%Demo.Accounts.User{hashed_password: hashed_password}, password) + when is_binary(hashed_password) and byte_size(password) > 0 do + Bcrypt.verify_pass(password, hashed_password) end def valid_password?(_, _) do diff --git a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs index f7b1ba3..3f696aa 100644 --- a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs @@ -6,7 +6,7 @@ defmodule Demo.Repo.Migrations.CreateUserAuthTables do create table(:users) do add :email, :citext, null: false - add :encrypted_password, :string, null: false + add :hashed_password, :string, null: false add :confirmed_at, :naive_datetime timestamps() end diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 9b1936f..7bffb68 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -87,7 +87,7 @@ defmodule Demo.AccountsTest do email = unique_user_email() {:ok, user} = Accounts.register_user(%{email: email, password: valid_user_password()}) assert user.email == email - assert is_binary(user.encrypted_password) + assert is_binary(user.hashed_password) assert is_nil(user.confirmed_at) end end From 97bd0035468c7a9797c0bf7128f29e0677fcae38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 1 Apr 2020 02:38:49 +0200 Subject: [PATCH 42/76] Update lib/demo_web/controllers/user_auth.ex Co-Authored-By: Aaron Tinio --- lib/demo_web/controllers/user_auth.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index ecafd39..1576edf 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -44,7 +44,7 @@ defmodule DemoWeb.UserAuth do # you must explicitly fetch the session data before clearing # and then immediately set it after clearing, for example: # - # def renew_session(conn) do + # defp renew_session(conn) do # preferred_locale = get_session(conn, :preferred_locale) # # conn From 7f3a55e9b7e93b96f3b6341468a2be7ad32e7b19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 1 Apr 2020 11:34:02 +0200 Subject: [PATCH 43/76] Apply suggestions from code review Co-Authored-By: Mark Murphy Co-Authored-By: Cassius Pacheco --- config/test.exs | 2 +- lib/demo/accounts/user.ex | 8 ++++---- .../controllers/user_reset_password_controller.ex | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/config/test.exs b/config/test.exs index c2e6703..ddab4c1 100644 --- a/config/test.exs +++ b/config/test.exs @@ -21,5 +21,5 @@ config :demo, DemoWeb.Endpoint, # Print only warnings and errors during test config :logger, level: :warn -# Only in tests, remove the complexity from the password encryption algorithm +# Only in tests, remove the complexity from the password hashing algorithm config :bcrypt_elixir, :log_rounds, 1 diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 9ab44ce..43b1559 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -17,7 +17,7 @@ defmodule Demo.Accounts.User do It is important to validate the length of both e-mail and password. Otherwise databases may truncate them without warnings, which could lead to unpredictable or insecure behaviour. Long passwords may also - be very expensive to encrypt. + be very expensive to hash. """ def registration_changeset(user, attrs) do user @@ -42,10 +42,10 @@ defmodule Demo.Accounts.User do # |> validate_format(:password, ~r/[a-z]/, message: "at least one lower case character") # |> validate_format(:password, ~r/[A-Z]/, message: "at least one upper case character") # |> validate_format(:password, ~r/[!?@#$%^&*_0-9]/, message: "at least one digit or punctuation character") - |> maybe_encrypt_password() + |> maybe_hash_password() end - defp maybe_encrypt_password(changeset) do + defp maybe_hash_password(changeset) do password = get_change(changeset, :password) if password && changeset.valid? do @@ -94,7 +94,7 @@ defmodule Demo.Accounts.User do Returns the given user if valid, If there is no user or the user doesn't have a password, - we encrypt a blank password to avoid timing attacks. + we hash a blank password to avoid timing attacks. """ def valid_password?(%Demo.Accounts.User{hashed_password: hashed_password}, password) when is_binary(hashed_password) and byte_size(password) > 0 do diff --git a/lib/demo_web/controllers/user_reset_password_controller.ex b/lib/demo_web/controllers/user_reset_password_controller.ex index c1f9f92..4581a8f 100644 --- a/lib/demo_web/controllers/user_reset_password_controller.ex +++ b/lib/demo_web/controllers/user_reset_password_controller.ex @@ -21,7 +21,7 @@ defmodule DemoWeb.UserResetPasswordController do conn |> put_flash( :info, - "If your e-mail is in our system, you receive instructions to reset your password shortly." + "If your e-mail is in our system, you will receive instructions to reset your password shortly." ) |> redirect(to: "/") end From f698bc830a2f48ac9009934ab28b89c92f285d68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 1 Apr 2020 02:45:11 +0200 Subject: [PATCH 44/76] Grammar --- lib/demo_web/controllers/user_auth.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index 1576edf..f3f4401 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -99,7 +99,7 @@ defmodule DemoWeb.UserAuth do end @doc """ - Used for routes that requires the user to not be authenticated. + Used for routes that require the user to not be authenticated. """ def redirect_if_user_is_authenticated(conn, _opts) do if conn.assigns[:current_user] do @@ -112,7 +112,7 @@ defmodule DemoWeb.UserAuth do end @doc """ - Used for routes that requires the user to be authenticated. + Used for routes that require the user to be authenticated. If you want to enforce the user e-mail is confirmed before they use the application at all, here would be a good place. From ef46ae1b24ed025a55af52adfd998a8be82410c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 1 Apr 2020 15:50:31 +0200 Subject: [PATCH 45/76] Update test/demo/accounts_test.exs Co-Authored-By: Jon Rowe --- test/demo/accounts_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 7bffb68..f49230b 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -78,7 +78,7 @@ defmodule Demo.AccountsTest do {:error, changeset} = Accounts.register_user(%{email: email}) assert "has already been taken" in errors_on(changeset).email - # Now try with the upcase e-mail too + # Now try with the upper cased e-mail too, to check that email case is ignored. {:error, changeset} = Accounts.register_user(%{email: String.upcase(email)}) assert "has already been taken" in errors_on(changeset).email end From d3aca054388917fc221d1526a8cfa72ba974667a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 1 Apr 2020 21:59:48 +0200 Subject: [PATCH 46/76] Apply suggestions from code review Co-Authored-By: Aaron Renner --- test/demo/accounts_test.exs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index f49230b..0034073 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -83,7 +83,7 @@ defmodule Demo.AccountsTest do assert "has already been taken" in errors_on(changeset).email end - test "registers users with an encrypted password" do + test "registers users with a hashed password" do email = unique_user_email() {:ok, user} = Accounts.register_user(%{email: email, password: valid_user_password()}) assert user.email == email @@ -290,7 +290,7 @@ defmodule Demo.AccountsTest do assert user_token = Repo.get_by(UserToken, token: token) assert user_token.context == "session" - # Creating the same token for another user fail + # Creating the same token for another user should fail assert_raise Ecto.ConstraintError, fn -> Repo.insert!(%UserToken{ token: user_token.token, From 326db9c730317dd145ff05c84a7599be6181ffb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 2 Apr 2020 10:10:36 +0200 Subject: [PATCH 47/76] More fixes --- lib/demo/accounts.ex | 2 +- lib/demo/accounts/user.ex | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 1d41f38..41e0a10 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -325,7 +325,7 @@ defmodule Demo.Accounts do end @doc """ - Returns an `%Ecto.Changeset{}` for tracking user reset password. + Resets the user password. ## Examples diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 43b1559..16d4ad2 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -93,8 +93,8 @@ defmodule Demo.Accounts.User do Returns the given user if valid, - If there is no user or the user doesn't have a password, - we hash a blank password to avoid timing attacks. + If there is no user or the user doesn't have a password, we call + `Bcrypt.no_user_verify/0` a blank password to avoid timing attacks. """ def valid_password?(%Demo.Accounts.User{hashed_password: hashed_password}, password) when is_binary(hashed_password) and byte_size(password) > 0 do @@ -102,7 +102,7 @@ defmodule Demo.Accounts.User do end def valid_password?(_, _) do - Bcrypt.hash_pwd_salt("unused hash to avoid timing attacks") + Bcrypt.no_user_verify() false end From 4abf899e35adc5d6157d1169309d9a474893061e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 2 Apr 2020 20:51:45 +0200 Subject: [PATCH 48/76] Apply suggestions from code review Co-Authored-By: Aaron Renner --- test/demo/accounts_test.exs | 2 +- test/demo_web/controllers/user_auth_test.exs | 2 +- test/demo_web/controllers/user_confirmation_controller_test.exs | 2 +- test/demo_web/controllers/user_registration_controller_test.exs | 2 +- .../controllers/user_reset_password_controller_test.exs | 2 +- test/demo_web/controllers/user_session_controller_test.exs | 2 +- test/demo_web/controllers/user_settings_controller_test.exs | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 0034073..c9be7a1 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -1,5 +1,5 @@ defmodule Demo.AccountsTest do - use Demo.DataCase + use Demo.DataCase, async: true import Demo.AccountsFixtures alias Demo.Accounts diff --git a/test/demo_web/controllers/user_auth_test.exs b/test/demo_web/controllers/user_auth_test.exs index da42420..6384d0d 100644 --- a/test/demo_web/controllers/user_auth_test.exs +++ b/test/demo_web/controllers/user_auth_test.exs @@ -1,5 +1,5 @@ defmodule DemoWeb.UserAuthTest do - use DemoWeb.ConnCase + use DemoWeb.ConnCase, async: true alias Demo.Accounts alias DemoWeb.UserAuth diff --git a/test/demo_web/controllers/user_confirmation_controller_test.exs b/test/demo_web/controllers/user_confirmation_controller_test.exs index 89cb66e..270d937 100644 --- a/test/demo_web/controllers/user_confirmation_controller_test.exs +++ b/test/demo_web/controllers/user_confirmation_controller_test.exs @@ -1,5 +1,5 @@ defmodule DemoWeb.UserConfirmationControllerTest do - use DemoWeb.ConnCase + use DemoWeb.ConnCase, async: true alias Demo.Accounts alias Demo.Repo diff --git a/test/demo_web/controllers/user_registration_controller_test.exs b/test/demo_web/controllers/user_registration_controller_test.exs index 77205c1..52f6ce7 100644 --- a/test/demo_web/controllers/user_registration_controller_test.exs +++ b/test/demo_web/controllers/user_registration_controller_test.exs @@ -1,5 +1,5 @@ defmodule DemoWeb.UserRegistrationControllerTest do - use DemoWeb.ConnCase + use DemoWeb.ConnCase, async: true import Demo.AccountsFixtures diff --git a/test/demo_web/controllers/user_reset_password_controller_test.exs b/test/demo_web/controllers/user_reset_password_controller_test.exs index 4699c8e..f64e2cc 100644 --- a/test/demo_web/controllers/user_reset_password_controller_test.exs +++ b/test/demo_web/controllers/user_reset_password_controller_test.exs @@ -1,5 +1,5 @@ defmodule DemoWeb.UserResetPasswordControllerTest do - use DemoWeb.ConnCase + use DemoWeb.ConnCase, async: true alias Demo.Accounts alias Demo.Repo diff --git a/test/demo_web/controllers/user_session_controller_test.exs b/test/demo_web/controllers/user_session_controller_test.exs index 3f97097..e40cc71 100644 --- a/test/demo_web/controllers/user_session_controller_test.exs +++ b/test/demo_web/controllers/user_session_controller_test.exs @@ -1,5 +1,5 @@ defmodule DemoWeb.UserSessionControllerTest do - use DemoWeb.ConnCase + use DemoWeb.ConnCase, async: true import Demo.AccountsFixtures diff --git a/test/demo_web/controllers/user_settings_controller_test.exs b/test/demo_web/controllers/user_settings_controller_test.exs index f9e0728..c3099b0 100644 --- a/test/demo_web/controllers/user_settings_controller_test.exs +++ b/test/demo_web/controllers/user_settings_controller_test.exs @@ -1,5 +1,5 @@ defmodule DemoWeb.UserSettingsControllerTest do - use DemoWeb.ConnCase + use DemoWeb.ConnCase, async: true alias Demo.Accounts import Demo.AccountsFixtures From f3a1571482561778a33e75d583cc327bb3fd556e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 9 Apr 2020 10:53:25 +0200 Subject: [PATCH 49/76] Update priv/repo/migrations/20200316103722_create_user_auth_tables.exs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Cristian Álvarez Belaustegui --- priv/repo/migrations/20200316103722_create_user_auth_tables.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs index 3f696aa..29887dc 100644 --- a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs @@ -18,7 +18,7 @@ defmodule Demo.Repo.Migrations.CreateUserAuthTables do add :token, :binary, null: false add :context, :string, null: false add :sent_to, :string - add :inserted_at, :naive_datetime + timestamps(updated_at: false) end create unique_index(:user_tokens, [:context, :token]) From 8655c008f4e98b96685474110f43b827f01f909a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 9 Apr 2020 10:55:46 +0200 Subject: [PATCH 50/76] Update lib/demo/accounts/user.ex --- lib/demo/accounts/user.ex | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 16d4ad2..bcfd484 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -15,9 +15,9 @@ defmodule Demo.Accounts.User do A user changeset for registration. It is important to validate the length of both e-mail and password. - Otherwise databases may truncate them without warnings, which could - lead to unpredictable or insecure behaviour. Long passwords may also - be very expensive to hash. + Otherwise databases may truncate the e-mail without warnings, which + could lead to unpredictable or insecure behaviour. Long passwords may + also be very expensive to hash for certain algorithms. """ def registration_changeset(user, attrs) do user From 5fbf0ba086868ca203ccfeaa92f43b4eb806d6f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 9 Apr 2020 12:08:58 +0200 Subject: [PATCH 51/76] Use distinct IDs --- lib/demo_web/templates/user_settings/edit.html.eex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/demo_web/templates/user_settings/edit.html.eex b/lib/demo_web/templates/user_settings/edit.html.eex index 1de8084..8b38ebc 100644 --- a/lib/demo_web/templates/user_settings/edit.html.eex +++ b/lib/demo_web/templates/user_settings/edit.html.eex @@ -14,7 +14,7 @@ <%= error_tag f, :email %> <%= label f, :current_password %> - <%= password_input f, :current_password, required: true, name: "current_password" %> + <%= password_input f, :current_password, required: true, name: "current_password", id: "current_password_for_email" %> <%= error_tag f, :current_password %>
@@ -40,7 +40,7 @@ <%= error_tag f, :password_confirmation %> <%= label f, :current_password %> - <%= password_input f, :current_password, required: true, name: "current_password" %> + <%= password_input f, :current_password, required: true, name: "current_password", id: "current_password_for_password" %> <%= error_tag f, :current_password %>
From 13de8522b59217361ca74e2f5e95d51ad880dbb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 9 Apr 2020 19:49:38 +0200 Subject: [PATCH 52/76] Delete password after hashing --- lib/demo/accounts/user.ex | 5 ++++- test/demo/accounts_test.exs | 12 ++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index bcfd484..215e954 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -2,6 +2,7 @@ defmodule Demo.Accounts.User do use Ecto.Schema import Ecto.Changeset + @derive {Inspect, except: [:password]} schema "users" do field :email, :string field :password, :string, virtual: true @@ -49,7 +50,9 @@ defmodule Demo.Accounts.User do password = get_change(changeset, :password) if password && changeset.valid? do - put_change(changeset, :hashed_password, Bcrypt.hash_pwd_salt(password)) + changeset + |> put_change(:hashed_password, Bcrypt.hash_pwd_salt(password)) + |> delete_change(:password) else changeset end diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index c9be7a1..8d872b9 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -89,6 +89,7 @@ defmodule Demo.AccountsTest do assert user.email == email assert is_binary(user.hashed_password) assert is_nil(user.confirmed_at) + assert is_nil(user.password) end end @@ -260,11 +261,12 @@ defmodule Demo.AccountsTest do end test "updates the password", %{user: user} do - {:ok, _} = + {:ok, user} = Accounts.update_user_password(user, valid_user_password(), %{ password: "new valid password" }) + assert is_nil(user.password) assert Accounts.get_user_by_email_and_password(user.email, "new valid password") end @@ -328,7 +330,7 @@ defmodule Demo.AccountsTest do user = user_fixture() token = Accounts.generate_session_token(user) assert Accounts.delete_session_token(token) == :ok - refute Accounts.get_user_by_session_token("oops") + refute Accounts.get_user_by_session_token(token) end end @@ -468,4 +470,10 @@ defmodule Demo.AccountsTest do refute Repo.get_by(UserToken, user_id: user.id) end end + + describe "inspect/2" do + test "does not include password" do + refute inspect(%User{password: "123456"}) =~ "password: \"123456\"" + end + end end From 6637c85d30adb77f4f2f3088f7f9753d1827c99c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 9 Apr 2020 20:11:15 +0200 Subject: [PATCH 53/76] Update test/demo/accounts_test.exs Co-Authored-By: Aaron Renner --- test/demo/accounts_test.exs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 8d872b9..536e028 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -460,7 +460,8 @@ defmodule Demo.AccountsTest do end test "updates the password", %{user: user} do - {:ok, _} = Accounts.reset_user_password(user, %{password: "new valid password"}) + {:ok, updated_user} = Accounts.reset_user_password(user, %{password: "new valid password"}) + assert is_nil(updated_user.password) assert Accounts.get_user_by_email_and_password(user.email, "new valid password") end From ead269e3d8af00f390e8bfbcbc8a97ab9e7afa9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 10 Apr 2020 00:30:00 +0200 Subject: [PATCH 54/76] Apply suggestions from code review Co-Authored-By: Aaron Renner --- lib/demo/accounts/user.ex | 2 +- test/demo/accounts_test.exs | 4 ++-- .../controllers/user_reset_password_controller_test.exs | 2 +- test/demo_web/controllers/user_settings_controller_test.exs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 215e954..801c9ea 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -79,7 +79,7 @@ defmodule Demo.Accounts.User do def password_changeset(user, attrs) do user |> cast(attrs, [:password]) - |> validate_confirmation(:password) + |> validate_confirmation(:password, message: "does not match password") |> validate_password() end diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 536e028..4cfb4dc 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -240,7 +240,7 @@ defmodule Demo.AccountsTest do assert %{ password: ["should be at least 12 character(s)"], - password_confirmation: ["does not match confirmation"] + password_confirmation: ["does not match password"] } = errors_on(changeset) end @@ -449,7 +449,7 @@ defmodule Demo.AccountsTest do assert %{ password: ["should be at least 12 character(s)"], - password_confirmation: ["does not match confirmation"] + password_confirmation: ["does not match password"] } = errors_on(changeset) end diff --git a/test/demo_web/controllers/user_reset_password_controller_test.exs b/test/demo_web/controllers/user_reset_password_controller_test.exs index f64e2cc..9d4e722 100644 --- a/test/demo_web/controllers/user_reset_password_controller_test.exs +++ b/test/demo_web/controllers/user_reset_password_controller_test.exs @@ -101,7 +101,7 @@ defmodule DemoWeb.UserResetPasswordControllerTest do response = html_response(conn, 200) assert response =~ "

Reset password

" assert response =~ "should be at least 12 character(s)" - assert response =~ "does not match confirmation" + assert response =~ "does not match password" end test "does not reset password with invalid token", %{conn: conn} do diff --git a/test/demo_web/controllers/user_settings_controller_test.exs b/test/demo_web/controllers/user_settings_controller_test.exs index c3099b0..1620e7c 100644 --- a/test/demo_web/controllers/user_settings_controller_test.exs +++ b/test/demo_web/controllers/user_settings_controller_test.exs @@ -50,7 +50,7 @@ defmodule DemoWeb.UserSettingsControllerTest do response = html_response(old_password_conn, 200) assert response =~ "

Settings

" assert response =~ "should be at least 12 character(s)" - assert response =~ "does not match confirmation" + assert response =~ "does not match password" assert response =~ "is not valid" assert get_session(old_password_conn, :user_token) == get_session(conn, :user_token) From c73cba441c29cc76996533e1295267cc54558c3b Mon Sep 17 00:00:00 2001 From: Serge Paquet Date: Fri, 10 Apr 2020 04:34:59 -0400 Subject: [PATCH 55/76] Add missing "for" attributes on password field labels (#2) Because the id of the password fields have been changed, the labels need their "for" attributes to match them. --- lib/demo_web/templates/user_settings/edit.html.eex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/demo_web/templates/user_settings/edit.html.eex b/lib/demo_web/templates/user_settings/edit.html.eex index 8b38ebc..f4dc065 100644 --- a/lib/demo_web/templates/user_settings/edit.html.eex +++ b/lib/demo_web/templates/user_settings/edit.html.eex @@ -13,7 +13,7 @@ <%= text_input f, :email, required: true %> <%= error_tag f, :email %> - <%= label f, :current_password %> + <%= label f, :current_password, for: "current_password_for_email" %> <%= password_input f, :current_password, required: true, name: "current_password", id: "current_password_for_email" %> <%= error_tag f, :current_password %> @@ -39,7 +39,7 @@ <%= password_input f, :password_confirmation, required: true %> <%= error_tag f, :password_confirmation %> - <%= label f, :current_password %> + <%= label f, :current_password, for: "current_password_for_password" %> <%= password_input f, :current_password, required: true, name: "current_password", id: "current_password_for_password" %> <%= error_tag f, :current_password %> From 01194c3e73515106460aae90f816ffdd3f4daa08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sat, 11 Apr 2020 18:21:31 +0200 Subject: [PATCH 56/76] token -> link --- lib/demo_web/controllers/user_confirmation_controller.ex | 2 +- lib/demo_web/controllers/user_reset_password_controller.ex | 2 +- lib/demo_web/controllers/user_settings_controller.ex | 2 +- .../controllers/user_confirmation_controller_test.exs | 4 ++-- .../controllers/user_reset_password_controller_test.exs | 4 ++-- test/demo_web/controllers/user_settings_controller_test.exs | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/demo_web/controllers/user_confirmation_controller.ex b/lib/demo_web/controllers/user_confirmation_controller.ex index 9d4eec6..c96b2fc 100644 --- a/lib/demo_web/controllers/user_confirmation_controller.ex +++ b/lib/demo_web/controllers/user_confirmation_controller.ex @@ -36,7 +36,7 @@ defmodule DemoWeb.UserConfirmationController do :error -> conn - |> put_flash(:error, "Confirmation token is invalid or it has expired.") + |> put_flash(:error, "Confirmation link is invalid or it has expired.") |> redirect(to: "/") end end diff --git a/lib/demo_web/controllers/user_reset_password_controller.ex b/lib/demo_web/controllers/user_reset_password_controller.ex index 4581a8f..320f62c 100644 --- a/lib/demo_web/controllers/user_reset_password_controller.ex +++ b/lib/demo_web/controllers/user_reset_password_controller.ex @@ -51,7 +51,7 @@ defmodule DemoWeb.UserResetPasswordController do conn |> assign(:user, user) |> assign(:token, token) else conn - |> put_flash(:error, "Reset password token is invalid or it has expired.") + |> put_flash(:error, "Reset password link is invalid or it has expired.") |> redirect(to: "/") |> halt() end diff --git a/lib/demo_web/controllers/user_settings_controller.ex b/lib/demo_web/controllers/user_settings_controller.ex index 649140b..2efc9e3 100644 --- a/lib/demo_web/controllers/user_settings_controller.ex +++ b/lib/demo_web/controllers/user_settings_controller.ex @@ -42,7 +42,7 @@ defmodule DemoWeb.UserSettingsController do :error -> conn - |> put_flash(:error, "Email change token is invalid or it has expired.") + |> put_flash(:error, "Email change link is invalid or it has expired.") |> redirect(to: Routes.user_settings_path(conn, :edit)) end end diff --git a/test/demo_web/controllers/user_confirmation_controller_test.exs b/test/demo_web/controllers/user_confirmation_controller_test.exs index 270d937..28b3677 100644 --- a/test/demo_web/controllers/user_confirmation_controller_test.exs +++ b/test/demo_web/controllers/user_confirmation_controller_test.exs @@ -71,13 +71,13 @@ defmodule DemoWeb.UserConfirmationControllerTest do conn = get(conn, Routes.user_confirmation_path(conn, :confirm, token)) assert redirected_to(conn) == "/" - assert get_flash(conn, :error) =~ "Confirmation token is invalid or it has expired" + assert get_flash(conn, :error) =~ "Confirmation link is invalid or it has expired" end test "does not confirm email with invalid token", %{conn: conn, user: user} do conn = get(conn, Routes.user_confirmation_path(conn, :confirm, "oops")) assert redirected_to(conn) == "/" - assert get_flash(conn, :error) =~ "Confirmation token is invalid or it has expired" + assert get_flash(conn, :error) =~ "Confirmation link is invalid or it has expired" refute Accounts.get_user!(user.id).confirmed_at end end diff --git a/test/demo_web/controllers/user_reset_password_controller_test.exs b/test/demo_web/controllers/user_reset_password_controller_test.exs index 9d4e722..959f305 100644 --- a/test/demo_web/controllers/user_reset_password_controller_test.exs +++ b/test/demo_web/controllers/user_reset_password_controller_test.exs @@ -60,7 +60,7 @@ defmodule DemoWeb.UserResetPasswordControllerTest do test "does not render reset password with invalid token", %{conn: conn} do conn = get(conn, Routes.user_reset_password_path(conn, :edit, "oops")) assert redirected_to(conn) == "/" - assert get_flash(conn, :error) =~ "Reset password token is invalid or it has expired" + assert get_flash(conn, :error) =~ "Reset password link is invalid or it has expired" end end @@ -107,7 +107,7 @@ defmodule DemoWeb.UserResetPasswordControllerTest do test "does not reset password with invalid token", %{conn: conn} do conn = put(conn, Routes.user_reset_password_path(conn, :update, "oops")) assert redirected_to(conn) == "/" - assert get_flash(conn, :error) =~ "Reset password token is invalid or it has expired" + assert get_flash(conn, :error) =~ "Reset password link is invalid or it has expired" end end end diff --git a/test/demo_web/controllers/user_settings_controller_test.exs b/test/demo_web/controllers/user_settings_controller_test.exs index 1620e7c..aa8a688 100644 --- a/test/demo_web/controllers/user_settings_controller_test.exs +++ b/test/demo_web/controllers/user_settings_controller_test.exs @@ -106,13 +106,13 @@ defmodule DemoWeb.UserSettingsControllerTest do conn = get(conn, Routes.user_settings_path(conn, :confirm_email, token)) assert redirected_to(conn) == "/users/settings" - assert get_flash(conn, :error) =~ "Email change token is invalid or it has expired" + assert get_flash(conn, :error) =~ "Email change link is invalid or it has expired" end test "does not update email with invalid token", %{conn: conn, user: user} do conn = get(conn, Routes.user_settings_path(conn, :confirm_email, "oops")) assert redirected_to(conn) == "/users/settings" - assert get_flash(conn, :error) =~ "Email change token is invalid or it has expired" + assert get_flash(conn, :error) =~ "Email change link is invalid or it has expired" assert Accounts.get_user_by_email(user.email) end From 288c67ac89fd7eec802a8632a68292b307018d78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 15 Apr 2020 15:59:20 +0200 Subject: [PATCH 57/76] Apply suggestions from code review Co-Authored-By: Wojtek Mach --- lib/demo/accounts.ex | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 41e0a10..a8dea2e 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -157,7 +157,7 @@ defmodule Demo.Accounts do ## Examples - iex> deliver_update_email_instructions(user, current_email, &Routes.user_update_email_url(conn, :edit)) + iex> deliver_update_email_instructions(user, current_email, &Routes.user_update_email_url(conn, :edit, &1)) {:ok, %{to: ..., body: ...}} """ @@ -248,7 +248,7 @@ defmodule Demo.Accounts do iex> deliver_user_confirmation_instructions(user, &Routes.user_confirmation_url(conn, :confirm)) {:ok, %{to: ..., body: ...}} - iex> deliver_user_confirmation_instructions(confirmed_user, &Routes.user_confirmation_url(conn, :confirm)) + iex> deliver_user_confirmation_instructions(confirmed_user, &Routes.user_confirmation_url(conn, :confirm, &1)) {:error, :already_confirmed} """ @@ -292,7 +292,7 @@ defmodule Demo.Accounts do ## Examples - iex> deliver_user_reset_password_instructions(user, &Routes.user_reset_password_url(conn, :edit)) + iex> deliver_user_reset_password_instructions(user, &Routes.user_reset_password_url(conn, :edit, &1)) {:ok, %{to: ..., body: ...}} """ From 9b1a27039e6cbfbdf95a070ad433960182fdf987 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 15 Apr 2020 20:38:40 +0200 Subject: [PATCH 58/76] Return user from confirm_user for convenience --- lib/demo/accounts.ex | 4 ++-- lib/demo_web/controllers/user_confirmation_controller.ex | 2 +- test/demo/accounts_test.exs | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index a8dea2e..10c14ca 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -272,8 +272,8 @@ defmodule Demo.Accounts do def confirm_user(token) do with {:ok, query} <- UserToken.verify_user_email_token_query(token, "confirm"), %User{} = user <- Repo.one(query), - {:ok, _} <- Repo.transaction(confirm_user_multi(user)) do - :ok + {:ok, %{user: user}} <- Repo.transaction(confirm_user_multi(user)) do + {:ok, user} else _ -> :error end diff --git a/lib/demo_web/controllers/user_confirmation_controller.ex b/lib/demo_web/controllers/user_confirmation_controller.ex index c96b2fc..580b616 100644 --- a/lib/demo_web/controllers/user_confirmation_controller.ex +++ b/lib/demo_web/controllers/user_confirmation_controller.ex @@ -29,7 +29,7 @@ defmodule DemoWeb.UserConfirmationController do # leaked token giving the user access to the account. def confirm(conn, %{"token" => token}) do case Accounts.confirm_user(token) do - :ok -> + {:ok, _} -> conn |> put_flash(:info, "Account confirmed successfully.") |> redirect(to: "/") diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 4cfb4dc..612f568 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -366,10 +366,10 @@ defmodule Demo.AccountsTest do end test "confirms the e-mail with a valid token", %{user: user, token: token} do - assert Accounts.confirm_user(token) == :ok - changed_user = Repo.get!(User, user.id) - assert changed_user.confirmed_at - assert changed_user.confirmed_at != user.confirmed_at + assert {:ok, confirmed_user} = Accounts.confirm_user(token) + assert confirmed_user.confirmed_at + assert confirmed_user.confirmed_at != user.confirmed_at + assert Repo.get!(User, user.id).confirmed_at refute Repo.get_by(UserToken, user_id: user.id) end From cd2ab05a3cbd15d90d99a1aa7c76226a31dc5df8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 16 Apr 2020 17:06:25 +0200 Subject: [PATCH 59/76] Apply suggestions from code review Co-Authored-By: Aaron Renner --- lib/demo/accounts/user_token.ex | 2 +- .../migrations/20200316103722_create_user_auth_tables.exs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex index fb1c30d..3a5088f 100644 --- a/lib/demo/accounts/user_token.ex +++ b/lib/demo/accounts/user_token.ex @@ -12,7 +12,7 @@ defmodule Demo.Accounts.UserToken do @change_email_validity_in_days 7 @session_validity_in_days 60 - schema "user_tokens" do + schema "users_tokens" do field :token, :binary field :context, :string field :sent_to, :string diff --git a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs index 29887dc..3d4269f 100644 --- a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_user_auth_tables.exs @@ -1,4 +1,4 @@ -defmodule Demo.Repo.Migrations.CreateUserAuthTables do +defmodule Demo.Repo.Migrations.CreateUsersAuthTables do use Ecto.Migration def change do @@ -13,7 +13,7 @@ defmodule Demo.Repo.Migrations.CreateUserAuthTables do create unique_index(:users, [:email]) - create table(:user_tokens) do + create table(:users_tokens) do add :user_id, references(:users, on_delete: :delete_all), null: false add :token, :binary, null: false add :context, :string, null: false @@ -21,6 +21,6 @@ defmodule Demo.Repo.Migrations.CreateUserAuthTables do timestamps(updated_at: false) end - create unique_index(:user_tokens, [:context, :token]) + create unique_index(:users_tokens, [:context, :token]) end end From 9912c79172c163d271ba6a58b49759076493fd4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 16 Apr 2020 17:09:50 +0200 Subject: [PATCH 60/76] Update file --- ...uth_tables.exs => 20200316103722_create_users_auth_tables.exs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename priv/repo/migrations/{20200316103722_create_user_auth_tables.exs => 20200316103722_create_users_auth_tables.exs} (100%) diff --git a/priv/repo/migrations/20200316103722_create_user_auth_tables.exs b/priv/repo/migrations/20200316103722_create_users_auth_tables.exs similarity index 100% rename from priv/repo/migrations/20200316103722_create_user_auth_tables.exs rename to priv/repo/migrations/20200316103722_create_users_auth_tables.exs From 25d083d105a406ab3a1c10ee7ab1b2bb4af31345 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 17 Apr 2020 01:07:03 +0200 Subject: [PATCH 61/76] Update test/demo_web/controllers/user_auth_test.exs Co-Authored-By: Michael Crumm --- test/demo_web/controllers/user_auth_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/demo_web/controllers/user_auth_test.exs b/test/demo_web/controllers/user_auth_test.exs index 6384d0d..af09668 100644 --- a/test/demo_web/controllers/user_auth_test.exs +++ b/test/demo_web/controllers/user_auth_test.exs @@ -139,7 +139,7 @@ defmodule DemoWeb.UserAuthTest do refute get_session(halted_conn, :user_return_to) end - test "does not redirect if user is not authenticated", %{conn: conn, user: user} do + test "does not redirect if user is authenticated", %{conn: conn, user: user} do conn = conn |> assign(:current_user, user) |> UserAuth.require_authenticated_user([]) refute conn.halted refute conn.status From fbd52ec105397cc63f3965a77b4aa7d17a2f573d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 17 Apr 2020 18:53:15 +0200 Subject: [PATCH 62/76] Update lib/demo/accounts/user.ex Co-Authored-By: Aaron Renner --- lib/demo/accounts/user.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 801c9ea..dbe9d52 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -97,7 +97,7 @@ defmodule Demo.Accounts.User do Returns the given user if valid, If there is no user or the user doesn't have a password, we call - `Bcrypt.no_user_verify/0` a blank password to avoid timing attacks. + `Bcrypt.no_user_verify/0` to avoid timing attacks. """ def valid_password?(%Demo.Accounts.User{hashed_password: hashed_password}, password) when is_binary(hashed_password) and byte_size(password) > 0 do From ecc8eb596e52fb041c3518d58d13503e2e25e5d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 17 Apr 2020 18:53:46 +0200 Subject: [PATCH 63/76] Update lib/demo/accounts.ex Co-Authored-By: Wojtek Mach --- lib/demo/accounts.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 10c14ca..e4b1425 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -245,7 +245,7 @@ defmodule Demo.Accounts do ## Examples - iex> deliver_user_confirmation_instructions(user, &Routes.user_confirmation_url(conn, :confirm)) + iex> deliver_user_confirmation_instructions(user, &Routes.user_confirmation_url(conn, :confirm, &1)) {:ok, %{to: ..., body: ...}} iex> deliver_user_confirmation_instructions(confirmed_user, &Routes.user_confirmation_url(conn, :confirm, &1)) From 6ae63abbe5c2e2c37f47dea83da1b830374ebf18 Mon Sep 17 00:00:00 2001 From: Aaron Renner Date: Tue, 28 Apr 2020 09:09:50 -0600 Subject: [PATCH 64/76] Only hash password when saving to database (#4) --- lib/demo/accounts/user.ex | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index dbe9d52..08df795 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -43,13 +43,11 @@ defmodule Demo.Accounts.User do # |> validate_format(:password, ~r/[a-z]/, message: "at least one lower case character") # |> validate_format(:password, ~r/[A-Z]/, message: "at least one upper case character") # |> validate_format(:password, ~r/[!?@#$%^&*_0-9]/, message: "at least one digit or punctuation character") - |> maybe_hash_password() + |> prepare_changes(&maybe_hash_password/1) end defp maybe_hash_password(changeset) do - password = get_change(changeset, :password) - - if password && changeset.valid? do + if password = get_change(changeset, :password) do changeset |> put_change(:hashed_password, Bcrypt.hash_pwd_salt(password)) |> delete_change(:password) From e51d52a735a5711d923cda9ff830b0fee17f335d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sat, 2 May 2020 00:41:22 +0200 Subject: [PATCH 65/76] Remove unused if --- lib/demo/accounts/user.ex | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 08df795..49cc524 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -47,13 +47,11 @@ defmodule Demo.Accounts.User do end defp maybe_hash_password(changeset) do - if password = get_change(changeset, :password) do - changeset - |> put_change(:hashed_password, Bcrypt.hash_pwd_salt(password)) - |> delete_change(:password) - else - changeset - end + password = get_change(changeset, :password) + + changeset + |> put_change(:hashed_password, Bcrypt.hash_pwd_salt(password)) + |> delete_change(:password) end @doc """ From bb13f6be8d5aacb35e609b976f1c28c5201a193c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sat, 2 May 2020 10:01:37 +0200 Subject: [PATCH 66/76] maybe_hash_password -> hash_passwword --- lib/demo/accounts/user.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 49cc524..4330de9 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -43,10 +43,10 @@ defmodule Demo.Accounts.User do # |> validate_format(:password, ~r/[a-z]/, message: "at least one lower case character") # |> validate_format(:password, ~r/[A-Z]/, message: "at least one upper case character") # |> validate_format(:password, ~r/[!?@#$%^&*_0-9]/, message: "at least one digit or punctuation character") - |> prepare_changes(&maybe_hash_password/1) + |> prepare_changes(&hash_password/1) end - defp maybe_hash_password(changeset) do + defp hash_password(changeset) do password = get_change(changeset, :password) changeset From 1eebcaed8913dad9854acce201777bd92f0d1b0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sun, 3 May 2020 17:09:42 +0200 Subject: [PATCH 67/76] Set the live_socket_id on login and disconnect on logout --- lib/demo_web/controllers/user_auth.ex | 10 ++++++++++ test/demo_web/controllers/user_auth_test.exs | 15 +++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index f3f4401..6ae86b6 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -18,6 +18,11 @@ defmodule DemoWeb.UserAuth do It renews the session ID and clears the whole session to avoid fixation attacks. See the renew_session function to customize this behaviour. + + It also sets a `:live_socket_id` key in the session, + so LiveView sessions are identified and automatically + disconnected on logout. The line can be safely removed + if you are not using LiveView. """ def login_user(conn, user, params \\ %{}) do token = Accounts.generate_session_token(user) @@ -26,6 +31,7 @@ defmodule DemoWeb.UserAuth do conn |> renew_session() |> put_session(:user_token, token) + |> put_session(:live_socket_id, "users_sessions:#{Base.url_encode64(token)}") |> maybe_write_remember_me_cookie(token, params) |> redirect(to: user_return_to || signed_in_path(conn)) end @@ -68,6 +74,10 @@ defmodule DemoWeb.UserAuth do user_token = get_session(conn, :user_token) user_token && Accounts.delete_session_token(user_token) + if live_socket_id = get_session(conn, :live_socket_id) do + DemoWeb.Endpoint.broadcast(live_socket_id, "disconnect", %{}) + end + conn |> renew_session() |> delete_resp_cookie(@remember_me_cookie) diff --git a/test/demo_web/controllers/user_auth_test.exs b/test/demo_web/controllers/user_auth_test.exs index af09668..3141589 100644 --- a/test/demo_web/controllers/user_auth_test.exs +++ b/test/demo_web/controllers/user_auth_test.exs @@ -18,6 +18,7 @@ defmodule DemoWeb.UserAuthTest do test "stores the user token in the session", %{conn: conn, user: user} do conn = UserAuth.login_user(conn, user) assert token = get_session(conn, :user_token) + assert get_session(conn, :live_socket_id) == "users_sessions:#{Base.url_encode64(token)}" assert redirected_to(conn) == "/" assert Accounts.get_user_by_session_token(token) end @@ -60,6 +61,20 @@ defmodule DemoWeb.UserAuthTest do refute Accounts.get_user_by_session_token(user_token) end + test "broadcasts to the given live_socket_id", %{conn: conn} do + live_socket_id = "users_sessions:abcdef-token" + Phoenix.PubSub.subscribe(Demo.PubSub, live_socket_id) + + conn + |> put_session(:live_socket_id, live_socket_id) + |> UserAuth.logout_user() + + assert_receive %Phoenix.Socket.Broadcast{ + event: "disconnect", + topic: "users_sessions:abcdef-token" + } + end + test "works even if user is already logged out", %{conn: conn} do conn = conn |> fetch_cookies() |> UserAuth.logout_user() refute get_session(conn, :user_token) From 4113391098d5bf1c562bb9e87d7e32a6f261b342 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 4 May 2020 13:06:53 +0200 Subject: [PATCH 68/76] Use the subscribe from the endpoint (undeprecated on v1.5.2) --- test/demo_web/controllers/user_auth_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/demo_web/controllers/user_auth_test.exs b/test/demo_web/controllers/user_auth_test.exs index 3141589..7ee8111 100644 --- a/test/demo_web/controllers/user_auth_test.exs +++ b/test/demo_web/controllers/user_auth_test.exs @@ -63,7 +63,7 @@ defmodule DemoWeb.UserAuthTest do test "broadcasts to the given live_socket_id", %{conn: conn} do live_socket_id = "users_sessions:abcdef-token" - Phoenix.PubSub.subscribe(Demo.PubSub, live_socket_id) + DemoWeb.Endpoint.subscribe(live_socket_id) conn |> put_session(:live_socket_id, live_socket_id) From 2b92a22ffa50203eaeceb8b5626a82c7b645f535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 7 May 2020 01:45:39 +0200 Subject: [PATCH 69/76] Remove unecessary user_ prefix --- lib/demo/accounts.ex | 12 ++++++------ lib/demo/accounts/user_token.ex | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index e4b1425..2ef1e7d 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -135,7 +135,7 @@ defmodule Demo.Accounts do def update_user_email(user, token) do context = "change:#{user.email}" - with {:ok, query} <- UserToken.verify_user_change_email_token_query(token, context), + with {:ok, query} <- UserToken.verify_change_email_token_query(token, context), %UserToken{sent_to: email} <- Repo.one(query), {:ok, _} <- Repo.transaction(user_email_multi(user, email, context)) do :ok @@ -164,7 +164,7 @@ defmodule Demo.Accounts do def deliver_update_email_instructions(%User{} = user, current_email, update_email_url_fun) when is_function(update_email_url_fun, 1) do {encoded_token, user_token} = - UserToken.build_user_email_token(user, "change:#{current_email}") + UserToken.build_email_token(user, "change:#{current_email}") Repo.insert!(user_token) UserNotifier.deliver_update_email_instructions(user, update_email_url_fun.(encoded_token)) @@ -257,7 +257,7 @@ defmodule Demo.Accounts do if user.confirmed_at do {:error, :already_confirmed} else - {encoded_token, user_token} = UserToken.build_user_email_token(user, "confirm") + {encoded_token, user_token} = UserToken.build_email_token(user, "confirm") Repo.insert!(user_token) UserNotifier.deliver_confirmation_instructions(user, confirmation_url_fun.(encoded_token)) end @@ -270,7 +270,7 @@ defmodule Demo.Accounts do and the token is deleted. """ def confirm_user(token) do - with {:ok, query} <- UserToken.verify_user_email_token_query(token, "confirm"), + with {:ok, query} <- UserToken.verify_email_token_query(token, "confirm"), %User{} = user <- Repo.one(query), {:ok, %{user: user}} <- Repo.transaction(confirm_user_multi(user)) do {:ok, user} @@ -298,7 +298,7 @@ defmodule Demo.Accounts do """ def deliver_user_reset_password_instructions(%User{} = user, reset_password_url_fun) when is_function(reset_password_url_fun, 1) do - {encoded_token, user_token} = UserToken.build_user_email_token(user, "reset_password") + {encoded_token, user_token} = UserToken.build_email_token(user, "reset_password") Repo.insert!(user_token) UserNotifier.deliver_reset_password_instructions(user, reset_password_url_fun.(encoded_token)) end @@ -316,7 +316,7 @@ defmodule Demo.Accounts do """ def get_user_by_reset_password_token(token) do - with {:ok, query} <- UserToken.verify_user_email_token_query(token, "reset_password"), + with {:ok, query} <- UserToken.verify_email_token_query(token, "reset_password"), %User{} = user <- Repo.one(query) do user else diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex index 3a5088f..34e90d6 100644 --- a/lib/demo/accounts/user_token.ex +++ b/lib/demo/accounts/user_token.ex @@ -54,7 +54,7 @@ defmodule Demo.Accounts.UserToken do The token is valid for a week as long as users don't change their email. """ - def build_user_email_token(user, context) do + def build_email_token(user, context) do build_hashed_token(user, context, user.email) end @@ -76,7 +76,7 @@ defmodule Demo.Accounts.UserToken do The query returns the user found by the token. """ - def verify_user_email_token_query(token, context) do + 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) @@ -103,7 +103,7 @@ defmodule Demo.Accounts.UserToken do The query returns the user found by the token. """ - def verify_user_change_email_token_query(token, context) do + def verify_change_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) From 300334227c4160b3a05ed4d0913c56a065014068 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 7 May 2020 02:04:36 +0200 Subject: [PATCH 70/76] Comments --- lib/demo/accounts/user.ex | 2 -- lib/demo/accounts/user_token.ex | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/demo/accounts/user.ex b/lib/demo/accounts/user.ex index 4330de9..3d394c4 100644 --- a/lib/demo/accounts/user.ex +++ b/lib/demo/accounts/user.ex @@ -90,8 +90,6 @@ defmodule Demo.Accounts.User do @doc """ Verifies the password. - Returns the given user if valid, - If there is no user or the user doesn't have a password, we call `Bcrypt.no_user_verify/0` to avoid timing attacks. """ diff --git a/lib/demo/accounts/user_token.ex b/lib/demo/accounts/user_token.ex index 34e90d6..ec4152f 100644 --- a/lib/demo/accounts/user_token.ex +++ b/lib/demo/accounts/user_token.ex @@ -101,7 +101,7 @@ defmodule Demo.Accounts.UserToken do @doc """ Checks if the token is valid and returns its underlying lookup query. - The query returns the user found by the token. + The query returns the user token record. """ def verify_change_email_token_query(token, context) do case Base.url_decode64(token, padding: false) do From 6f2ef5df3eb56cae99183192eef5bd84c1c8e607 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 8 May 2020 12:38:55 +0200 Subject: [PATCH 71/76] Logout should succeed even if the user already logged out --- lib/demo_web/router.ex | 2 +- .../controllers/user_session_controller_test.exs | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/demo_web/router.ex b/lib/demo_web/router.ex index 4ec47b1..bc6df60 100644 --- a/lib/demo_web/router.ex +++ b/lib/demo_web/router.ex @@ -45,7 +45,6 @@ defmodule DemoWeb.Router do scope "/", DemoWeb do pipe_through [:browser, :require_authenticated_user] - delete "/users/logout", UserSessionController, :delete get "/users/settings", UserSettingsController, :edit put "/users/settings/update_password", UserSettingsController, :update_password put "/users/settings/update_email", UserSettingsController, :update_email @@ -55,6 +54,7 @@ defmodule DemoWeb.Router do scope "/", DemoWeb do pipe_through [:browser] + delete "/users/logout", UserSessionController, :delete get "/users/confirm", UserConfirmationController, :new post "/users/confirm", UserConfirmationController, :create get "/users/confirm/:token", UserConfirmationController, :confirm diff --git a/test/demo_web/controllers/user_session_controller_test.exs b/test/demo_web/controllers/user_session_controller_test.exs index e40cc71..f0b1c56 100644 --- a/test/demo_web/controllers/user_session_controller_test.exs +++ b/test/demo_web/controllers/user_session_controller_test.exs @@ -67,16 +67,18 @@ defmodule DemoWeb.UserSessionControllerTest do end describe "DELETE /users/logout" do - test "redirects if not logged in", %{conn: conn} do - conn = delete(conn, Routes.user_session_path(conn, :delete)) - assert redirected_to(conn) == "/users/login" - end - test "logs the user out", %{conn: conn, user: user} do conn = conn |> login_user(user) |> delete(Routes.user_session_path(conn, :delete)) assert redirected_to(conn) == "/" refute get_session(conn, :user_token) assert get_flash(conn, :info) =~ "Logged out successfully" end + + test "succeeds even if the user is not logged in", %{conn: conn} do + conn = delete(conn, Routes.user_session_path(conn, :delete)) + assert redirected_to(conn) == "/" + refute get_session(conn, :user_token) + assert get_flash(conn, :info) =~ "Logged out successfully" + end end end From 83d5deb33a9da42fd949d794f735773870882767 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 8 May 2020 16:20:31 +0200 Subject: [PATCH 72/76] Update lib/demo/accounts.ex Co-authored-by: Aaron Renner --- lib/demo/accounts.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 2ef1e7d..68f63ca 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -163,8 +163,7 @@ defmodule Demo.Accounts do """ def deliver_update_email_instructions(%User{} = user, current_email, update_email_url_fun) when is_function(update_email_url_fun, 1) do - {encoded_token, user_token} = - UserToken.build_email_token(user, "change:#{current_email}") + {encoded_token, user_token} = UserToken.build_email_token(user, "change:#{current_email}") Repo.insert!(user_token) UserNotifier.deliver_update_email_instructions(user, update_email_url_fun.(encoded_token)) From 78cb46ecc535878aec7a0255b9079a4cb4a42b27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sun, 17 May 2020 11:29:45 +0200 Subject: [PATCH 73/76] Add user prefix to session token functions --- lib/demo/accounts.ex | 4 ++-- lib/demo_web/controllers/user_auth.ex | 4 ++-- test/demo/accounts_test.exs | 16 ++++++++-------- test/demo_web/controllers/user_auth_test.exs | 6 +++--- test/support/conn_case.ex | 2 +- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 68f63ca..02bae8a 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -215,7 +215,7 @@ defmodule Demo.Accounts do @doc """ Generates a session token. """ - def generate_session_token(user) do + def generate_user_session_token(user) do {token, user_token} = UserToken.build_session_token(user) Repo.insert!(user_token) token @@ -232,7 +232,7 @@ defmodule Demo.Accounts do @doc """ Deletes the signed token with the given context. """ - def delete_session_token(token) do + def delete_user_session_token(token) do Repo.delete_all(UserToken.token_and_context_query(token, "session")) :ok end diff --git a/lib/demo_web/controllers/user_auth.ex b/lib/demo_web/controllers/user_auth.ex index 6ae86b6..883337b 100644 --- a/lib/demo_web/controllers/user_auth.ex +++ b/lib/demo_web/controllers/user_auth.ex @@ -25,7 +25,7 @@ defmodule DemoWeb.UserAuth do if you are not using LiveView. """ def login_user(conn, user, params \\ %{}) do - token = Accounts.generate_session_token(user) + token = Accounts.generate_user_session_token(user) user_return_to = get_session(conn, :user_return_to) conn @@ -72,7 +72,7 @@ defmodule DemoWeb.UserAuth do """ def logout_user(conn) do user_token = get_session(conn, :user_token) - user_token && Accounts.delete_session_token(user_token) + user_token && Accounts.delete_user_session_token(user_token) if live_socket_id = get_session(conn, :live_socket_id) do DemoWeb.Endpoint.broadcast(live_socket_id, "disconnect", %{}) diff --git a/test/demo/accounts_test.exs b/test/demo/accounts_test.exs index 612f568..e694eee 100644 --- a/test/demo/accounts_test.exs +++ b/test/demo/accounts_test.exs @@ -271,7 +271,7 @@ defmodule Demo.AccountsTest do end test "deletes all tokens for the given user", %{user: user} do - _ = Accounts.generate_session_token(user) + _ = Accounts.generate_user_session_token(user) {:ok, _} = Accounts.update_user_password(user, valid_user_password(), %{ @@ -282,13 +282,13 @@ defmodule Demo.AccountsTest do end end - describe "generate_session_token/1" do + describe "generate_user_session_token/1" do setup do %{user: user_fixture()} end test "generates a token", %{user: user} do - token = Accounts.generate_session_token(user) + token = Accounts.generate_user_session_token(user) assert user_token = Repo.get_by(UserToken, token: token) assert user_token.context == "session" @@ -306,7 +306,7 @@ defmodule Demo.AccountsTest do describe "get_user_by_session_token/1" do setup do user = user_fixture() - token = Accounts.generate_session_token(user) + token = Accounts.generate_user_session_token(user) %{user: user, token: token} end @@ -325,11 +325,11 @@ defmodule Demo.AccountsTest do end end - describe "delete_session_token/1" do + describe "delete_user_session_token/1" do test "deletes the token" do user = user_fixture() - token = Accounts.generate_session_token(user) - assert Accounts.delete_session_token(token) == :ok + token = Accounts.generate_user_session_token(user) + assert Accounts.delete_user_session_token(token) == :ok refute Accounts.get_user_by_session_token(token) end end @@ -466,7 +466,7 @@ defmodule Demo.AccountsTest do end test "deletes all tokens for the given user", %{user: user} do - _ = Accounts.generate_session_token(user) + _ = Accounts.generate_user_session_token(user) {:ok, _} = Accounts.reset_user_password(user, %{password: "new valid password"}) refute Repo.get_by(UserToken, user_id: user.id) end diff --git a/test/demo_web/controllers/user_auth_test.exs b/test/demo_web/controllers/user_auth_test.exs index 7ee8111..97f63fb 100644 --- a/test/demo_web/controllers/user_auth_test.exs +++ b/test/demo_web/controllers/user_auth_test.exs @@ -45,7 +45,7 @@ defmodule DemoWeb.UserAuthTest do describe "logout_user/1" do test "erases session and cookies", %{conn: conn, user: user} do - user_token = Accounts.generate_session_token(user) + user_token = Accounts.generate_user_session_token(user) conn = conn @@ -85,7 +85,7 @@ defmodule DemoWeb.UserAuthTest do describe "fetch_current_user/2" do test "authenticates user from session", %{conn: conn, user: user} do - user_token = Accounts.generate_session_token(user) + user_token = Accounts.generate_user_session_token(user) conn = conn |> put_session(:user_token, user_token) |> UserAuth.fetch_current_user([]) assert conn.assigns.current_user.id == user.id end @@ -107,7 +107,7 @@ defmodule DemoWeb.UserAuthTest do end test "does not authenticate if data is missing", %{conn: conn, user: user} do - _ = Accounts.generate_session_token(user) + _ = Accounts.generate_user_session_token(user) conn = UserAuth.fetch_current_user(conn, []) refute get_session(conn, :user_token) refute conn.assigns.current_user diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 09d8764..ee31b29 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -60,7 +60,7 @@ defmodule DemoWeb.ConnCase do It returns an updated `conn`. """ def login_user(conn, user) do - token = Demo.Accounts.generate_session_token(user) + token = Demo.Accounts.generate_user_session_token(user) conn |> Phoenix.ConnTest.init_test_session(%{}) From 57bf7df63e270027d0c69d01c948390c2ee5fb27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 May 2020 09:16:10 +0200 Subject: [PATCH 74/76] Add index on user_id --- priv/repo/migrations/20200316103722_create_users_auth_tables.exs | 1 + 1 file changed, 1 insertion(+) diff --git a/priv/repo/migrations/20200316103722_create_users_auth_tables.exs b/priv/repo/migrations/20200316103722_create_users_auth_tables.exs index 3d4269f..d3a10d7 100644 --- a/priv/repo/migrations/20200316103722_create_users_auth_tables.exs +++ b/priv/repo/migrations/20200316103722_create_users_auth_tables.exs @@ -21,6 +21,7 @@ defmodule Demo.Repo.Migrations.CreateUsersAuthTables do timestamps(updated_at: false) end + create index(:users_tokens, [:user_id]) create unique_index(:users_tokens, [:context, :token]) end end From ed0e210016d536fb5bc4e6400fb23cc014a4ac48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 26 May 2020 09:06:11 +0200 Subject: [PATCH 75/76] Update lib/demo/accounts.ex Co-authored-by: Aaron Renner --- lib/demo/accounts.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/demo/accounts.ex b/lib/demo/accounts.ex index 02bae8a..14ca3a9 100644 --- a/lib/demo/accounts.ex +++ b/lib/demo/accounts.ex @@ -183,7 +183,7 @@ defmodule Demo.Accounts do end @doc """ - Returns an `%Ecto.Changeset{}` for changing the user password. + Updates the user password. ## Examples From 4664c376273af7100e31766ccf2d76bc7cf153e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 26 May 2020 15:37:31 +0200 Subject: [PATCH 76/76] Update test/support/fixtures/accounts_fixtures.ex Co-authored-by: Aaron Renner --- test/support/fixtures/accounts_fixtures.ex | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/support/fixtures/accounts_fixtures.ex b/test/support/fixtures/accounts_fixtures.ex index c3a38fd..ec18348 100644 --- a/test/support/fixtures/accounts_fixtures.ex +++ b/test/support/fixtures/accounts_fixtures.ex @@ -1,4 +1,9 @@ defmodule Demo.AccountsFixtures do + @moduledoc """ + This module defines test helpers for creating + entities via the `Demo.Accounts` context. + """ + def unique_user_email, do: "user#{System.unique_integer()}@example.com" def valid_user_password, do: "hello world!"