From fa15a25044343fb0173dce0a04eddc1d3b587a6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Floure?= Date: Fri, 8 Jan 2021 14:41:37 +0100 Subject: [PATCH] Wire/test registration and password reset --- config/config.exs | 3 +- lib/recycledcloud/accounts.ex | 19 +++---- lib/recycledcloud/accounts/user.ex | 34 ++++++++++--- .../user_registration_controller.ex | 49 ++++++++++++------- .../user_reset_password_controller.ex | 7 ++- .../templates/user_registration/new.html.eex | 5 ++ .../user_reset_password/edit.html.eex | 8 +-- test/recycledcloud/accounts_test.exs | 10 ++-- .../user_reset_password_controller_test.exs | 16 +++--- .../user_settings_controller_test.exs | 10 ++-- test/support/ldap_test_environment.ex | 2 +- 11 files changed, 102 insertions(+), 61 deletions(-) diff --git a/config/config.exs b/config/config.exs index 0bbc0b1..bdcb2e1 100644 --- a/config/config.exs +++ b/config/config.exs @@ -9,7 +9,8 @@ use Mix.Config config :recycledcloud, namespace: RecycledCloud, - ecto_repos: [RecycledCloud.Repo] + ecto_repos: [RecycledCloud.Repo], + enable_registration: true # Configures the endpoint config :recycledcloud, RecycledCloudWeb.Endpoint, diff --git a/lib/recycledcloud/accounts.ex b/lib/recycledcloud/accounts.ex index 9e68c55..089a036 100644 --- a/lib/recycledcloud/accounts.ex +++ b/lib/recycledcloud/accounts.ex @@ -89,12 +89,9 @@ defmodule RecycledCloud.Accounts do case User.register(user) do {:ok, result} -> {:ok, result} {:error, :entryAlreadyExists} -> - { - :error, - Ecto.Changeset.add_error( - changeset, :username, "has already been taken" - ) - } + err = changeset + |> Ecto.Changeset.add_error(:username, "has already been taken") + |> Ecto.Changeset.apply_action(:update) end err -> err end @@ -216,7 +213,11 @@ defmodule RecycledCloud.Accounts do Repo.delete_all(UserToken.user_and_contexts_query(user, :all)) {:ok, user} {:error, err} -> - changeset_errors = [current_password: {"Unknown error: #{err}", [err: inspect(err)]}] + msg = {"Unknown error: #{err}", [err: inspect(err)]} + changeset_errors = [ + current_password: msg, + password: msg + ] updated_changeset = changeset |> Map.put(:action, :update) |> Map.put(:errors, changeset_errors) @@ -244,7 +245,7 @@ defmodule RecycledCloud.Accounts do |> User.password_changeset(attrs) |> User.validate_current_password(current_password) - set_user_password(user, changeset, attrs[:password]) + set_user_password(user, changeset, attrs["password"]) end ## Session @@ -376,7 +377,7 @@ defmodule RecycledCloud.Accounts do changeset = user |> User.password_changeset(attrs) - set_user_password(user, changeset, attrs[:password]) + set_user_password(user, changeset, attrs["password"]) end ## Keys diff --git a/lib/recycledcloud/accounts/user.ex b/lib/recycledcloud/accounts/user.ex index 0a51a93..4da5b3c 100644 --- a/lib/recycledcloud/accounts/user.ex +++ b/lib/recycledcloud/accounts/user.ex @@ -54,15 +54,33 @@ defmodule RecycledCloud.Accounts.User do end end + defp get_last_ldap_uid() do + query = fn ldap_conn -> + LDAP.search(ldap_conn, :objectClass, 'posixAccount') + end + + case query |> LDAP.execute do + {:ok, entries} -> + uids = Enum.map(entries, fn entry -> + entry.uidNumber |> Enum.at(0) |> List.to_integer + end) + + {:ok, uids |> Enum.max()} + {:error, err} -> {:error, err} + end + end + defp create_ldap_user(%User{} = user) do - # FIXME: read dn template from conf, gid_number - # FIXME: generate uidNumber + # TODO: read dn template from conf, gid_number conf = Application.get_env(:recycledcloud, :ldap) basetree = conf |> Keyword.get(:base_dn) + default_group_gid = conf |> Keyword.get(:default_group_gid, 1000) + dn = ("uid=" <> user.username <> ",ou=Users," <> basetree) |> String.to_charlist - uid_number = 1234 - gid_number = 1000 + {:ok, last_uid_number} = get_last_ldap_uid() + uid_number = last_uid_number + 1 + gid_number = default_group_gid ldif = [ {'uid', [user.username]}, @@ -222,12 +240,14 @@ defmodule RecycledCloud.Accounts.User do end end - def set_password(%User{} = user, new_password) - when byte_size(new_password) > 0 do + def set_password(%User{dn: nil} = _user, _), do: {:error, "User DN must be set"} + def set_password(_, nil), do: {:error, "Password cannot be nil"} + def set_password(%User{dn: dn} = _user, new_password) when + byte_size(new_password) > 0do query = fn ldap_conn -> :eldap.modify_password( ldap_conn, - to_charlist(user.dn), + to_charlist(dn), to_charlist(new_password) ) end diff --git a/lib/recycledcloud_web/controllers/user_registration_controller.ex b/lib/recycledcloud_web/controllers/user_registration_controller.ex index 3a251ac..f8bba99 100644 --- a/lib/recycledcloud_web/controllers/user_registration_controller.ex +++ b/lib/recycledcloud_web/controllers/user_registration_controller.ex @@ -5,33 +5,46 @@ defmodule RecycledCloudWeb.UserRegistrationController do alias RecycledCloud.Accounts.User alias RecycledCloudWeb.UserAuth + defp is_registration_enabled?() do + Application.get_env(:recycledcloud, :enable_registration) + end + def new(conn, _params) do changeset = Accounts.change_user_registration(%User{}) - conn - |> put_flash(:error, "Registration is disabled for the time being.") + + with_notice = unless is_registration_enabled?() do + conn + |> put_flash(:error, "Registration is disabled for the time being.") + else + conn + end + + with_notice |> render("new.html", changeset: changeset) end def create(conn, %{"user" => user_params}) do changeset = Accounts.change_user_registration(%User{}) - conn - |> redirect(to: Routes.user_registration_path(conn, :new)) + unless is_registration_enabled?() do + conn + |> redirect(to: Routes.user_registration_path(conn, :new)) + else + case Accounts.register_user(user_params) do + {:ok, user} -> + {:ok, _} = + Accounts.deliver_user_confirmation_instructions( + user, + &Routes.user_confirmation_url(conn, :confirm, &1) + ) - #case Accounts.register_user(user_params) do - # {:ok, user} -> - # {:ok, _} = - # Accounts.deliver_user_confirmation_instructions( - # user, - # &Routes.user_confirmation_url(conn, :confirm, &1) - # ) + conn + |> put_flash(:info, "User created successfully.") + |> UserAuth.log_in_user(user) - # conn - # |> put_flash(:info, "User created successfully.") - # |> UserAuth.log_in_user(user) - - # {:error, %Ecto.Changeset{} = changeset} -> - # render(conn, "new.html", changeset: changeset) - #end + {:error, %Ecto.Changeset{} = changeset} -> + render(conn, "new.html", changeset: changeset) + end + end end end diff --git a/lib/recycledcloud_web/controllers/user_reset_password_controller.ex b/lib/recycledcloud_web/controllers/user_reset_password_controller.ex index f364e86..76df16f 100644 --- a/lib/recycledcloud_web/controllers/user_reset_password_controller.ex +++ b/lib/recycledcloud_web/controllers/user_reset_password_controller.ex @@ -21,7 +21,7 @@ defmodule RecycledCloudWeb.UserResetPasswordController do conn |> put_flash( :info, - "If your email is in our system, you will receive instructions to reset your password shortly." + "If your account is in our system, you will receive instructions to reset your password shortly." ) |> redirect(to: "/") end @@ -33,7 +33,10 @@ defmodule RecycledCloudWeb.UserResetPasswordController do # Do not log in 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_user_password(conn.assigns.user, user_params) do + # FIXME: understand why conn.assigns.user is not populated with LDAP + # attributes. + user = conn.assigns.user.id |> Accounts.User.get!() + case Accounts.reset_user_password(user, user_params) do {:ok, _} -> conn |> put_flash(:info, "Password reset successfully.") diff --git a/lib/recycledcloud_web/templates/user_registration/new.html.eex b/lib/recycledcloud_web/templates/user_registration/new.html.eex index 9d0c59d..74c2cfa 100644 --- a/lib/recycledcloud_web/templates/user_registration/new.html.eex +++ b/lib/recycledcloud_web/templates/user_registration/new.html.eex @@ -12,6 +12,11 @@
+ <%= text_input f, :email, required: true, placeholder: "E-mail" %> + <%= error_tag f, :email %> + +
+ <%= password_input f, :password, required: true, placeholder: "Password" %> <%= error_tag f, :password %> diff --git a/lib/recycledcloud_web/templates/user_reset_password/edit.html.eex b/lib/recycledcloud_web/templates/user_reset_password/edit.html.eex index 94a550a..2d08658 100644 --- a/lib/recycledcloud_web/templates/user_reset_password/edit.html.eex +++ b/lib/recycledcloud_web/templates/user_reset_password/edit.html.eex @@ -7,12 +7,12 @@ <% end %> - <%= label f, :password, "New password" %> - <%= password_input f, :password, required: true %> + <%= password_input f, :password, required: true, placeholder: "New password" %> <%= error_tag f, :password %> - <%= label f, :password_confirmation, "Confirm new password" %> - <%= password_input f, :password_confirmation, required: true %> +
+ + <%= password_input f, :password_confirmation, required: true, placeholder: "New password confirmation" %> <%= error_tag f, :password_confirmation %>
diff --git a/test/recycledcloud/accounts_test.exs b/test/recycledcloud/accounts_test.exs index ff84905..857447e 100644 --- a/test/recycledcloud/accounts_test.exs +++ b/test/recycledcloud/accounts_test.exs @@ -262,14 +262,14 @@ defmodule RecycledCloud.AccountsTest do too_long = String.duplicate("db", 100) {:error, changeset} = - Accounts.update_user_password(user, valid_user_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_user_password()}) + Accounts.update_user_password(user, "invalid", %{"password" => valid_user_password()}) assert %{current_password: ["is not valid"]} = errors_on(changeset) end @@ -277,7 +277,7 @@ defmodule RecycledCloud.AccountsTest do test "updates the password", %{user: user} do {:ok, user} = Accounts.update_user_password(user, valid_user_password(), %{ - password: "new valid password" + "password" => "new valid password" }) assert is_nil(user.password) @@ -289,7 +289,7 @@ defmodule RecycledCloud.AccountsTest do {:ok, _} = Accounts.update_user_password(user, valid_user_password(), %{ - password: "new valid password" + "password" => "new valid password" }) refute Repo.get_by(UserToken, user_id: user.id) @@ -481,7 +481,7 @@ defmodule RecycledCloud.AccountsTest do test "deletes all tokens for the given user", %{user: user} do _ = Accounts.generate_user_session_token(user) - {:ok, _} = Accounts.reset_user_password(user, %{password: "new valid password"}) + {:ok, _} = Accounts.reset_user_password(user, %{"password" => "new valid password"}) refute Repo.get_by(UserToken, user_id: user.id) end end diff --git a/test/recycledcloud_web/controllers/user_reset_password_controller_test.exs b/test/recycledcloud_web/controllers/user_reset_password_controller_test.exs index 08071e0..3b40fdd 100644 --- a/test/recycledcloud_web/controllers/user_reset_password_controller_test.exs +++ b/test/recycledcloud_web/controllers/user_reset_password_controller_test.exs @@ -22,22 +22,22 @@ defmodule RecycledCloudWeb.UserResetPasswordControllerTest do 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} + "user" => %{"username" => user.username} }) assert redirected_to(conn) == "/" - assert get_flash(conn, :info) =~ "If your email is in our system" + assert get_flash(conn, :info) =~ "If your account 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 + test "does not send reset password token if username is invalid", %{conn: conn} do conn = post(conn, Routes.user_reset_password_path(conn, :create), %{ - "user" => %{"email" => "unknown@example.com"} + "user" => %{"username" => "unknown"} }) assert redirected_to(conn) == "/" - assert get_flash(conn, :info) =~ "If your email is in our system" + assert get_flash(conn, :info) =~ "If your account is in our system" assert Repo.all(Accounts.UserToken) == [] end end @@ -86,21 +86,21 @@ defmodule RecycledCloudWeb.UserResetPasswordControllerTest do assert redirected_to(conn) == Routes.user_session_path(conn, :new) 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") + assert Accounts.get_user_by_username_and_password(user.username, "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" => "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 =~ "should be at least 6 character(s)" assert response =~ "does not match password" end diff --git a/test/recycledcloud_web/controllers/user_settings_controller_test.exs b/test/recycledcloud_web/controllers/user_settings_controller_test.exs index 5b91fa6..69d3ef9 100644 --- a/test/recycledcloud_web/controllers/user_settings_controller_test.exs +++ b/test/recycledcloud_web/controllers/user_settings_controller_test.exs @@ -10,7 +10,7 @@ defmodule RecycledCloudWeb.UserSettingsControllerTest 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

" + assert response =~ "

Account settings

" end test "redirects if user is not logged in" do @@ -70,7 +70,7 @@ defmodule RecycledCloudWeb.UserSettingsControllerTest do assert redirected_to(conn) == Routes.user_settings_path(conn, :edit) assert get_flash(conn, :info) =~ "A link to confirm your email" - assert Accounts.get_user_by_email(user.email) + assert Accounts.get_user_by_username(user.username).email == user.email end test "does not update email on invalid data", %{conn: conn} do @@ -82,7 +82,6 @@ defmodule RecycledCloudWeb.UserSettingsControllerTest do }) response = html_response(conn, 200) - assert response =~ "

Settings

" assert response =~ "must have the @ sign and no spaces" assert response =~ "is not valid" end @@ -104,8 +103,7 @@ defmodule RecycledCloudWeb.UserSettingsControllerTest do conn = get(conn, Routes.user_settings_path(conn, :confirm_email, token)) assert redirected_to(conn) == Routes.user_settings_path(conn, :edit) assert get_flash(conn, :info) =~ "Email changed successfully" - refute Accounts.get_user_by_email(user.email) - assert Accounts.get_user_by_email(email) + assert Accounts.get_user_by_username(user.username).email == email conn = get(conn, Routes.user_settings_path(conn, :confirm_email, token)) assert redirected_to(conn) == Routes.user_settings_path(conn, :edit) @@ -116,7 +114,7 @@ defmodule RecycledCloudWeb.UserSettingsControllerTest do conn = get(conn, Routes.user_settings_path(conn, :confirm_email, "oops")) assert redirected_to(conn) == Routes.user_settings_path(conn, :edit) assert get_flash(conn, :error) =~ "Email change link is invalid or it has expired" - assert Accounts.get_user_by_email(user.email) + assert Accounts.get_user_by_username(user.username) end test "redirects if user is not logged in", %{token: token} do diff --git a/test/support/ldap_test_environment.ex b/test/support/ldap_test_environment.ex index ffcbeb4..2828b5b 100644 --- a/test/support/ldap_test_environment.ex +++ b/test/support/ldap_test_environment.ex @@ -23,7 +23,7 @@ defmodule RecycledCloud.LDAPTestEnvironment do :ok -> # Wait for LDAP server to be populated. # FIXME: poll state instead of taking a nap! - Process.sleep(1000) + Process.sleep(2000) {:ok, port, container} :timeout -> {:error, :timeout}