diff --git a/lib/birdy_chat/identity.ex b/lib/birdy_chat/identity.ex index cbb502f..3c62f1b 100644 --- a/lib/birdy_chat/identity.ex +++ b/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] diff --git a/lib/birdy_chat/message.ex b/lib/birdy_chat/message.ex index d3adba7..a28e00c 100644 --- a/lib/birdy_chat/message.ex +++ b/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() diff --git a/lib/birdy_chat_web.ex b/lib/birdy_chat_web.ex index 6d766a2..f38f95c 100644 --- a/lib/birdy_chat_web.ex +++ b/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()) diff --git a/lib/birdy_chat_web/api/messages/controller.ex b/lib/birdy_chat_web/api/messages/controller.ex index ea86b9b..ee0693b 100644 --- a/lib/birdy_chat_web/api/messages/controller.ex +++ b/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 diff --git a/lib/birdy_chat_web/api/messages/json.ex b/lib/birdy_chat_web/api/messages/json.ex index 74613fa..194bb7e 100644 --- a/lib/birdy_chat_web/api/messages/json.ex +++ b/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 diff --git a/lib/birdy_chat_web/api/server/internal/controller.ex b/lib/birdy_chat_web/api/server/internal/controller.ex index 4d6ed90..cdb9766 100644 --- a/lib/birdy_chat_web/api/server/internal/controller.ex +++ b/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 - case BirdyChat.Message.validate(params) do - {:ok, changeset} -> - case BirdyChat.MessageWriter.write(changeset.changes) do - :ok -> - conn - |> put_status(:created) - |> render(:create, message: changeset.changes) - end - end - else + with true <- authorised?(conn.req_headers, params), + {:ok, changeset} <- BirdyChat.Message.validate_for_inter_server_use(params), + :ok <- BirdyChat.MessageWriter.write(changeset.changes) do conn - |> put_status(:forbidden) - |> render(:error, message: "Unauthorised") + |> put_status(:created) + |> render(:create, message: changeset.changes) + else + _any -> + conn + |> put_status(:forbidden) + |> render(:error, message: "Unauthorised") end end diff --git a/lib/birdy_chat_web/api/server/internal/json.ex b/lib/birdy_chat_web/api/server/internal/json.ex index c45c2a0..56352d7 100644 --- a/lib/birdy_chat_web/api/server/internal/json.ex +++ b/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 diff --git a/lib/birdy_chat_web/components/core_components.ex b/lib/birdy_chat_web/components/core_components.ex index b50a31b..59a56ef 100644 --- a/lib/birdy_chat_web/components/core_components.ex +++ b/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 -> - Phoenix.HTML.Form.normalize_value("checkbox", assigns[:value]) - end) + assign_new(assigns, :checked, fn -> Form.normalize_value("checkbox", assigns[:value]) end) ~H"""
diff --git a/rel/overlays/bin/server b/rel/overlays/bin/server deleted file mode 100755 index 8a9148e..0000000 --- a/rel/overlays/bin/server +++ /dev/null @@ -1,5 +0,0 @@ -#!/bin/sh -set -eu - -cd -P -- "$(dirname -- "$0")" -PHX_SERVER=true exec ./birdy_chat start diff --git a/rel/overlays/bin/server_1 b/rel/overlays/bin/server_1 new file mode 100755 index 0000000..4fbccde --- /dev/null +++ b/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 diff --git a/rel/overlays/bin/server_2 b/rel/overlays/bin/server_2 new file mode 100755 index 0000000..a565979 --- /dev/null +++ b/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 diff --git a/test/birdy_chat_web/api/messages_test.exs b/test/birdy_chat_web/api/messages_test.exs index e1c7cd1..e945288 100644 --- a/test/birdy_chat_web/api/messages_test.exs +++ b/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 - message = %{"from" => "test2-user", "to" => "test1-user", "message" => "123"} + test "returns error when you post a message to other server", %{conn: conn, url: url} do + 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", - "to" => "test1-user", + "from" => "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 - message = %{"from" => "test2-user", "to" => "test1-user", "message" => "123"} + test "writes message to file", %{conn: conn, url: url, username: username, path: path} 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", - "to" => "test1-user", + "from" => "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 - message = %{"from" => "test2-user", "to" => "test1-user", "message" => "123"} + test "appends message to file", %{conn: conn, url: url, username: username, path: path} do + 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 diff --git a/test/support/channel_case.ex b/test/support/channel_case.ex deleted file mode 100644 index 0cd840c..0000000 --- a/test/support/channel_case.ex +++ /dev/null @@ -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