Docs and cleanup

lib/birdy_chat/identity.ex

@@ -1,4 +1,15 @@
 defmodule BirdyChat.Identity do
+  @moduledoc """
+  Server identity, populated on startup from the following environment variables:
+
+  - BIRDY_CHAT_IDENTITY: Name of the server, a string that can be formatted into an integer.
+  - BIRDY_CHAT_PEERS: List of peer servers in format of
+    `1::http://localhost:4001;2::http://localhost:4002`
+
+  When the environment variables are not present the server starts in test mode and can only
+  communicate with itself.
+  """
+
   use Agent
 
   defstruct [:identity, :peers, :mode]

lib/birdy_chat/message.ex

@@ -1,4 +1,9 @@
 defmodule BirdyChat.Message do
+  @moduledoc """
+  Main module for input validation. I decided to re-use Ecto because of existence of Phoenix.Ecto
+  that clearly integrates the error messages produced from Ecto into HTTP plumbing of Phoenix.
+  """
+
   use Ecto.Schema
 
   embedded_schema do
@@ -9,12 +14,20 @@ defmodule BirdyChat.Message do
     field :server, :string
   end
 
-  def validate(params) do
+  @doc """
+  Validation for inter-server communication. It is essentially the same as validate/1
+  but without the requirement to communicate with home server only.
+
+  This can be also designed as a function that accepts other function or some configuration option
+  but two separately named functions are easier to understand and less prone to misuse.
+  """
+  def validate_for_inter_server_use(params) do
     changeset =
       %__MODULE__{}
       |> Ecto.Changeset.cast(params, [:from, :to, :message])
       |> Ecto.Changeset.validate_required([:from, :to, :message])
       |> put_routing()
+      |> validate_is_local()
 
     if changeset.valid? do
       {:ok, changeset}
@@ -23,6 +36,45 @@ defmodule BirdyChat.Message do
     end
   end
 
+  def validate(params) do
+    changeset =
+      %__MODULE__{}
+      |> Ecto.Changeset.cast(params, [:from, :to, :message])
+      |> Ecto.Changeset.validate_required([:from, :to, :message])
+      |> put_routing()
+      |> validate_home_server()
+
+    if changeset.valid? do
+      {:ok, changeset}
+    else
+      {:error, changeset}
+    end
+  end
+
+  defp validate_is_local(%Ecto.Changeset{changes: %{routing: :local}} = changeset) do
+    changeset
+  end
+
+  defp validate_is_local(%Ecto.Changeset{} = changeset) do
+    changeset
+    |> Ecto.Changeset.add_error(:from, "you can only communicate with your home server")
+  end
+
+  defp validate_home_server(%Ecto.Changeset{changes: %{from: from}} = changeset) do
+    identity = BirdyChat.Identity.identity()
+
+    if String.starts_with?(from, identity) do
+      changeset
+    else
+      changeset
+      |> Ecto.Changeset.add_error(:from, "you can only communicate with your home server")
+    end
+  end
+
+  defp validate_home_server(%Ecto.Changeset{} = changeset) do
+    changeset
+  end
+
   defp put_routing(%Ecto.Changeset{changes: %{to: to}} = changeset) do
     identity = BirdyChat.Identity.identity()
 

lib/birdy_chat_web.ex

@@ -88,8 +88,8 @@ defmodule BirdyChatWeb do
       import BirdyChatWeb.CoreComponents
 
       # Common modules used in templates
-      alias Phoenix.LiveView.JS
       alias BirdyChatWeb.Layouts
+      alias Phoenix.LiveView.JS
 
       # Routes generation with the ~p sigil
       unquote(verified_routes())

lib/birdy_chat_web/api/messages/controller.ex

@@ -1,4 +1,8 @@
 defmodule BirdyChatWeb.Api.Messages.Controller do
+  @moduledoc """
+  The endpoint to be used by users from the "home server".
+  """
+
   use BirdyChatWeb, :controller
 
   def create(conn, params) do

lib/birdy_chat_web/api/messages/json.ex

@@ -1,4 +1,6 @@
 defmodule BirdyChatWeb.Api.Messages.JSON do
+  @moduledoc false
+
   def render("create.json", %{message: message}) do
     message
   end

lib/birdy_chat_web/api/server/internal/controller.ex

@@ -1,21 +1,36 @@
 defmodule BirdyChatWeb.Api.Server.Internal.Controller do
+  @moduledoc """
+  A controller for handling inter-server communication. It started off with using Erlang term
+  format instead of JSON as communication language but then I removed it for the following
+  reasons:
+
+  1. The messages are mostly binaries anyway, there is no big efficiency gain from skipping JSON.
+  2. Testing JSON is much easier than testing erlang term format.
+  3. Erlang term format can give an illusion of extra security but unless the transport is HTTPS
+  then the communication is still inherently unsafe.
+  4. Erlang term format is difficult to handle for unfamiliar developers, you need to remember
+  about safe conversion to avoid atom exhaustion attacks or sending an `rm -rf /` function over
+  the wire.
+
+  The endpoint is protected by simple authentication that requires the secret key of all servers
+  being the same. It is good enough for a demo, but for any real application it would need to be
+  reconsidered.
+  """
+
   use BirdyChatWeb, :controller
 
   def create(conn, params) do
-    if authorised?(conn.req_headers, params) do
+    with true <- authorised?(conn.req_headers, params),
-      case BirdyChat.Message.validate(params) do
+         {:ok, changeset} <- BirdyChat.Message.validate_for_inter_server_use(params),
-        {:ok, changeset} ->
+         :ok <- BirdyChat.MessageWriter.write(changeset.changes) do
-          case BirdyChat.MessageWriter.write(changeset.changes) do
-            :ok ->
-              conn
-              |> put_status(:created)
-              |> render(:create, message: changeset.changes)
-          end
-      end
-    else
       conn
-      |> put_status(:forbidden)
+      |> put_status(:created)
-      |> render(:error, message: "Unauthorised")
+      |> render(:create, message: changeset.changes)
+    else
+      _any ->
+        conn
+        |> put_status(:forbidden)
+        |> render(:error, message: "Unauthorised")
     end
   end
 

lib/birdy_chat_web/api/server/internal/json.ex

@@ -1,4 +1,6 @@
 defmodule BirdyChatWeb.Api.Server.Internal.JSON do
+  @moduledoc false
+
   def render("create.json", %{message: message}) do
     message
   end

lib/birdy_chat_web/components/core_components.ex

@@ -29,6 +29,7 @@ defmodule BirdyChatWeb.CoreComponents do
   use Phoenix.Component
   use Gettext, backend: BirdyChatWeb.Gettext
 
+  alias Phoenix.HTML.Form
   alias Phoenix.LiveView.JS
 
   @doc """
@@ -200,9 +201,7 @@ defmodule BirdyChatWeb.CoreComponents do
 
   def input(%{type: "checkbox"} = assigns) do
     assigns =
-      assign_new(assigns, :checked, fn ->
+      assign_new(assigns, :checked, fn -> Form.normalize_value("checkbox", assigns[:value]) end)
-        Phoenix.HTML.Form.normalize_value("checkbox", assigns[:value])
-      end)
 
     ~H"""
     <div class="fieldset mb-2">

rel/overlays/bin/server

@@ -1,5 +0,0 @@
-#!/bin/sh
-set -eu
-
-cd -P -- "$(dirname -- "$0")"
-PHX_SERVER=true exec ./birdy_chat start

rel/overlays/bin/server_1

@@ -0,0 +1,12 @@
+#!/bin/sh
+set -eu
+
+cd -P -- "$(dirname -- "$0")"
+
+export SECRET_KEY_BASE=Yhmq6FzYQt4g5AFHfSdMBKKf4oRo4KRo703FK6b7RwmH5pXlyQNompUOF7/EEC5t
+export BIRDY_CHAT_PORT=4001
+export BIRDY_CHAT_IDENTITY=1
+export BIRDY_CHAT_PEERS=2::http://localhost:4002
+export PHX_SERVER=true
+export RELEASE_NAME=server_1
+exec ./birdy_chat start

rel/overlays/bin/server_2

@@ -0,0 +1,12 @@
+#!/bin/sh
+set -eu
+
+cd -P -- "$(dirname -- "$0")"
+
+export SECRET_KEY_BASE=Yhmq6FzYQt4g5AFHfSdMBKKf4oRo4KRo703FK6b7RwmH5pXlyQNompUOF7/EEC5t
+export BIRDY_CHAT_PORT=4002
+export BIRDY_CHAT_IDENTITY=2
+export BIRDY_CHAT_PEERS=1::http://localhost:4001
+export PHX_SERVER=true
+export RELEASE_NAME=server_2
+exec ./birdy_chat start

test/birdy_chat_web/api/messages_test.exs

@@ -4,14 +4,17 @@ defmodule BirdyChatWeb.Api.MessagesTest do
   setup %{conn: conn} do
     url = ~p"/api/messages"
 
-    path = Application.app_dir(:birdy_chat, ["priv", "messages", "test1-user.txt"])
+    unique_user_id = System.unique_integer([:positive])
+    username = "test1-user#{unique_user_id}"
+
+    path = Application.app_dir(:birdy_chat, ["priv", "messages", "#{username}.txt"])
     on_exit(fn -> File.rm(path) end)
 
     conn =
       conn
       |> put_req_header("content-type", "application/json")
 
-    %{conn: conn, url: url}
+    %{conn: conn, url: url, username: username, path: path}
   end
 
   describe "POST /api/messages to other server" do
@@ -92,16 +95,30 @@ defmodule BirdyChatWeb.Api.MessagesTest do
       assert result == expected_result
     end
 
-    test "returns message and 201 when successful", %{conn: conn, url: url} do
+    test "returns error when you post a message to other server", %{conn: conn, url: url} do
-      message = %{"from" => "test2-user", "to" => "test1-user", "message" => "123"}
+      message = %{"from" => "nothomeserver-user", "to" => "test1-user", "message" => "123"}
+
+      payload = Jason.encode!(message)
+      conn = post(conn, url, payload)
+      assert result = json_response(conn, :unprocessable_entity)
+
+      expected_result = %{
+        "errors" => %{"from" => ["you can only communicate with your home server"]}
+      }
+
+      assert result == expected_result
+    end
+
+    test "returns message and 201 when successful", %{conn: conn, url: url, username: username} do
+      message = %{"from" => "test1-user", "to" => username, "message" => "123"}
 
       payload = Jason.encode!(message)
       conn = post(conn, url, payload)
       assert result = json_response(conn, :created)
 
       expected_result = %{
-        "from" => "test2-user",
+        "from" => "test1-user",
-        "to" => "test1-user",
+        "to" => username,
         "message" => "123",
         "routing" => "local",
         "server" => "test1"
@@ -110,15 +127,15 @@ defmodule BirdyChatWeb.Api.MessagesTest do
       assert result == expected_result
     end
 
-    test "writes message to file", %{conn: conn, url: url} do
+    test "writes message to file", %{conn: conn, url: url, username: username, path: path} do
-      message = %{"from" => "test2-user", "to" => "test1-user", "message" => "123"}
+      message = %{"from" => "test1-user", "to" => username, "message" => "123"}
       payload = Jason.encode!(message)
       conn = post(conn, url, payload)
       assert result = json_response(conn, :created)
 
       expected_result = %{
-        "from" => "test2-user",
+        "from" => "test1-user",
-        "to" => "test1-user",
+        "to" => username,
         "message" => "123",
         "routing" => "local",
         "server" => "test1"
@@ -126,24 +143,22 @@ defmodule BirdyChatWeb.Api.MessagesTest do
 
       assert result == expected_result
 
-      path = Application.app_dir(:birdy_chat, ["priv", "messages", "test1-user.txt"])
       contents = File.read!(path)
-      assert contents == "test2-user: 123\n"
+      assert contents == "test1-user: 123\n"
     end
 
-    test "appends message to file", %{conn: conn, url: url} do
+    test "appends message to file", %{conn: conn, url: url, username: username, path: path} do
-      message = %{"from" => "test2-user", "to" => "test1-user", "message" => "123"}
+      message = %{"from" => "test1-user", "to" => username, "message" => "123"}
       payload = Jason.encode!(message)
       post(conn, url, payload)
 
-      message = %{"from" => "test2-user", "to" => "test1-user", "message" => "456"}
+      message = %{"from" => "test1-user", "to" => username, "message" => "456"}
       payload = Jason.encode!(message)
       conn = post(conn, url, payload)
       assert json_response(conn, :created)
 
-      path = Application.app_dir(:birdy_chat, ["priv", "messages", "test1-user.txt"])
       contents = File.read!(path)
-      assert contents == "test2-user: 123\ntest2-user: 456\n"
+      assert contents == "test1-user: 123\ntest1-user: 456\n"
     end
   end
 end

test/support/channel_case.ex

@@ -1,34 +0,0 @@
-defmodule BirdyChatWeb.ChannelCase do
-  @moduledoc """
-  This module defines the test case to be used by
-  channel tests.
-
-  Such tests rely on `Phoenix.ChannelTest` and also
-  import other functionality to make it easier
-  to build common data structures and query the data layer.
-
-  Finally, if the test case interacts with the database,
-  we enable the SQL sandbox, so changes done to the database
-  are reverted at the end of every test. If you are using
-  PostgreSQL, you can even run database tests asynchronously
-  by setting `use BirdyChatWeb.ChannelCase, async: true`, although
-  this option is not recommended for other databases.
-  """
-
-  use ExUnit.CaseTemplate
-
-  using do
-    quote do
-      # Import conveniences for testing with channels
-      import Phoenix.ChannelTest
-      import BirdyChatWeb.ChannelCase
-
-      # The default endpoint for testing
-      @endpoint BirdyChatWeb.Endpoint
-    end
-  end
-
-  setup _tags do
-    :ok
-  end
-end