Add more register tests

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: <<Enum.random(@alphabet)>>
+  def random_string(length), do: random_string(length, @alphabet)
+
+  def random_string(length, alphabet) when length >= 1 do
+    for _ <- 1..length, into: "", do: <<Enum.random(alphabet)>>
   end
 end

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

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)

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
 

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."},

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