From 6b9c9333b818887dab5ebf00b89baa6e3a183a81 Mon Sep 17 00:00:00 2001 From: Pim Kunis Date: Tue, 13 Jul 2021 19:35:02 +0200 Subject: [PATCH] Add more register tests --- lib/matrix_server.ex | 6 +- lib/matrix_server/account.ex | 25 ++++--- lib/matrix_server_web/api/register.ex | 2 +- .../controllers/auth_controller.ex | 13 +--- lib/matrix_server_web/plug/error.ex | 2 +- test/controllers/auth_controller_test.exs | 71 ++++++++++++++++--- 6 files changed, 86 insertions(+), 33 deletions(-) diff --git a/lib/matrix_server.ex b/lib/matrix_server.ex index 0e302ea..3734b87 100644 --- a/lib/matrix_server.ex +++ b/lib/matrix_server.ex @@ -24,7 +24,9 @@ defmodule MatrixServer do def localpart_regex, do: ~r/^([a-z0-9\._=\/])+$/ @alphabet Enum.into(?a..?z, []) ++ Enum.into(?A..?Z, []) - def random_string(length) when length >= 1 do - for _ <- 1..length, into: "", do: <> + def random_string(length), do: random_string(length, @alphabet) + + def random_string(length, alphabet) when length >= 1 do + for _ <- 1..length, into: "", do: <> end end diff --git a/lib/matrix_server/account.ex b/lib/matrix_server/account.ex index 835c60f..7cba00d 100644 --- a/lib/matrix_server/account.ex +++ b/lib/matrix_server/account.ex @@ -4,6 +4,7 @@ defmodule MatrixServer.Account do import Ecto.{Changeset, Query} alias MatrixServer.{Repo, Account, Device} + alias MatrixServerWeb.API.Register alias Ecto.Multi @max_mxid_length 255 @@ -32,20 +33,22 @@ defmodule MatrixServer.Account do end end - def register(params) do - Multi.new() - |> Multi.insert(:account, changeset(%Account{}, params)) - |> Multi.insert(:device, fn %{account: account} -> - device_id = Device.generate_device_id(account.localpart) + def register(%Register{} = api) do + account_params = %{ + localpart: api.username || MatrixServer.random_string(10, ?a..?z), + password_hash: Bcrypt.hash_pwd_salt(api.password) + } - params = - Map.update(params, :device_id, device_id, fn - nil -> device_id - x -> x - end) + Multi.new() + |> Multi.insert(:account, changeset(%Account{}, account_params)) + |> Multi.insert(:device, fn %{account: account} -> + device_params = %{ + display_name: api.initial_device_display_name, + device_id: api.device_id || Device.generate_device_id(account.localpart) + } Ecto.build_assoc(account, :devices) - |> Device.changeset(params) + |> Device.changeset(device_params) end) |> Multi.run(:device_with_access_token, &Device.insert_new_access_token/2) end diff --git a/lib/matrix_server_web/api/register.ex b/lib/matrix_server_web/api/register.ex index 4291979..00d3345 100644 --- a/lib/matrix_server_web/api/register.ex +++ b/lib/matrix_server_web/api/register.ex @@ -23,7 +23,7 @@ defmodule MatrixServerWeb.API.Register do :username, :inhibit_login ]) - |> validate_required([:password, :username]) + |> validate_required([:password]) end def get_error(%Changeset{errors: [error | _]}), do: get_error(error) diff --git a/lib/matrix_server_web/controllers/auth_controller.ex b/lib/matrix_server_web/controllers/auth_controller.ex index b100580..077a8bc 100644 --- a/lib/matrix_server_web/controllers/auth_controller.ex +++ b/lib/matrix_server_web/controllers/auth_controller.ex @@ -14,20 +14,14 @@ defmodule MatrixServerWeb.AuthController do def register(conn, %{"auth" => %{"type" => @register_type}} = params) do case Register.changeset(params) do %Changeset{valid?: true} = cs -> - # TODO: refactor this - input = - apply_changes(cs) - |> Map.from_struct() - |> MatrixServer.maybe_update_map(:initial_device_display_name, :display_name) - |> MatrixServer.maybe_update_map(:username, :localpart) - |> MatrixServer.maybe_update_map(:password, :password_hash, &Bcrypt.hash_pwd_salt/1) + api = apply_changes(cs) - case Account.register(input) |> Repo.transaction() do + case Account.register(api) |> Repo.transaction() do {:ok, %{device_with_access_token: device}} -> data = %{user_id: MatrixServer.get_mxid(device.localpart)} data = - if not input.inhibit_login do + if not api.inhibit_login do data |> Map.put(:device_id, device.device_id) |> Map.put(:access_token, device.access_token) @@ -40,7 +34,6 @@ defmodule MatrixServerWeb.AuthController do |> json(data) {:error, _, cs, _} -> - IO.inspect(cs) put_error(conn, Register.get_error(cs)) end diff --git a/lib/matrix_server_web/plug/error.ex b/lib/matrix_server_web/plug/error.ex index ad2d5db..9a53e5c 100644 --- a/lib/matrix_server_web/plug/error.ex +++ b/lib/matrix_server_web/plug/error.ex @@ -4,7 +4,7 @@ defmodule MatrixServerWeb.Plug.Error do @error_code_and_message %{ bad_json: {400, "M_BAD_JSON", "Bad request."}, - user_in_use: {400, "M_USE_IN_USE", "Username is already taken."}, + user_in_use: {400, "M_USER_IN_USE", "Username is already taken."}, invalid_username: {400, "M_INVALID_USERNAME", "Invalid username."}, forbidden: {400, "M_FORBIDDEN", "The requested action is forbidden."}, unrecognized: {400, "M_UNRECOGNIZED", "Unrecognized request."}, diff --git a/test/controllers/auth_controller_test.exs b/test/controllers/auth_controller_test.exs index 4e2abd8..83f8b81 100644 --- a/test/controllers/auth_controller_test.exs +++ b/test/controllers/auth_controller_test.exs @@ -1,8 +1,17 @@ defmodule MatrixServerWeb.AuthControllerTest do use MatrixServerWeb.ConnCase + import Ecto.Query + + alias MatrixServer.{Repo, Device, Factory} alias MatrixServerWeb.Endpoint + @basic_params %{ + "username" => "user", + "password" => "lemmein", + "auth" => %{"type" => "m.login.dummy"} + } + describe "register endpoint" do test "renders the auth flow when no auth parameter is given", %{conn: conn} do conn = post(conn, Routes.auth_path(conn, :register)) @@ -12,18 +21,64 @@ defmodule MatrixServerWeb.AuthControllerTest do end test "registers account with minimal information", %{conn: conn} do - params = %{ - "username" => "user", - "password" => "lemmein", - "auth" => %{"type" => "m.login.dummy"} - } - - conn = post_json(conn, Routes.auth_path(Endpoint, :register), params) - + conn = post_json(conn, Routes.auth_path(Endpoint, :register), @basic_params) user_id = MatrixServer.get_mxid("user") assert %{"access_token" => _, "device_id" => _, "user_id" => ^user_id} = json_response(conn, 200) end + + test "registers and sets device id", %{conn: conn} do + params = Map.put(@basic_params, :device_id, "android") + conn = post_json(conn, Routes.auth_path(Endpoint, :register), params) + + assert %{"device_id" => "android"} = json_response(conn, 200) + end + + test "registers and sets display name", %{conn: conn} do + params = Map.put(@basic_params, :initial_device_display_name, "My Android") + conn = post_json(conn, Routes.auth_path(Endpoint, :register), params) + + assert json_response(conn, 200) + assert Repo.one!(from d in Device, select: d.display_name) == "My Android" + end + + test "rejects account if localpart is already in use", %{conn: conn} do + Factory.insert(:account, localpart: "sneed") + + conn = + post_json(conn, Routes.auth_path(Endpoint, :register), %{ + @basic_params + | "username" => "sneed" + }) + + assert %{"errcode" => "M_USER_IN_USE"} = json_response(conn, 400) + end + + test "obeys inhibit_login parameter", %{conn: conn} do + params = Map.put(@basic_params, :inhibit_login, "true") + conn = post_json(conn, Routes.auth_path(Endpoint, :register), params) + + assert response = json_response(conn, 200) + refute Map.has_key?(response, "access_token") + refute Map.has_key?(response, "device_id") + end + + test "generates localpart if omitted", %{conn: conn} do + params = Map.delete(@basic_params, "username") + conn = post_json(conn, Routes.auth_path(Endpoint, :register), params) + + assert %{"user_id" => _} = json_response(conn, 200) + end + + test "rejects invalid usernames", %{conn: conn} do + conn = + post_json(conn, Routes.auth_path(Endpoint, :register), %{ + @basic_params + | "username" => "User1" + }) + + assert %{"errcode" => "M_INVALID_USERNAME"} = json_response(conn, 400) + end end end