From f760c6060bfe557bf4b3f1e98a358e18f3462414 Mon Sep 17 00:00:00 2001 From: Alex Reichert Date: Wed, 7 Apr 2021 16:25:28 -0400 Subject: [PATCH] Support specifying customer in `POST /api/v1/messages` endpoint (#723) * Support specifying a customer_id in POST /api/v1/messages * Improve error handling --- lib/chat_api/customers.ex | 2 +- .../controllers/fallback_controller.ex | 22 ++++ .../controllers/message_controller.ex | 48 +++++-- .../controllers/message_controller_test.exs | 123 +++++++++++++++++- 4 files changed, 184 insertions(+), 11 deletions(-) diff --git a/lib/chat_api/customers.ex b/lib/chat_api/customers.ex index 75219a4b8..21d428f10 100644 --- a/lib/chat_api/customers.ex +++ b/lib/chat_api/customers.ex @@ -45,7 +45,7 @@ defmodule ChatApi.Customers do |> Repo.paginate(pagination_params) end - @spec get_customer!(binary(), atom() | list(atom()) | keyword()) :: Customer.t() | nil + @spec get_customer!(binary(), atom() | list(atom()) | keyword()) :: Customer.t() def get_customer!(id, preloads \\ [:company, :tags]) do Customer |> Repo.get!(id) diff --git a/lib/chat_api_web/controllers/fallback_controller.ex b/lib/chat_api_web/controllers/fallback_controller.ex index 1cd5356a6..828185dbc 100644 --- a/lib/chat_api_web/controllers/fallback_controller.ex +++ b/lib/chat_api_web/controllers/fallback_controller.ex @@ -35,4 +35,26 @@ defmodule ChatApiWeb.FallbackController do } }) end + + def call(conn, {:error, :unprocessable_entity, message}) do + conn + |> put_status(422) + |> json(%{ + error: %{ + status: 422, + message: message + } + }) + end + + def call(conn, {:error, :forbidden, message}) do + conn + |> put_status(403) + |> json(%{ + error: %{ + status: 403, + message: message + } + }) + end end diff --git a/lib/chat_api_web/controllers/message_controller.ex b/lib/chat_api_web/controllers/message_controller.ex index aa721713e..b07c24aad 100644 --- a/lib/chat_api_web/controllers/message_controller.ex +++ b/lib/chat_api_web/controllers/message_controller.ex @@ -7,13 +7,13 @@ defmodule ChatApiWeb.MessageController do action_fallback(ChatApiWeb.FallbackController) - plug :authorize when action in [:show, :update, :delete] + plug(:authorize when action in [:show, :update, :delete]) defp authorize(conn, _) do id = conn.path_params["id"] - with %{id: user_id, account_id: account_id} <- conn.assigns.current_user, - message = %{account_id: ^account_id, user_id: ^user_id} <- Messages.get_message!(id) do + with %{id: _user_id, account_id: account_id} <- conn.assigns.current_user, + message = %{account_id: ^account_id} <- Messages.get_message!(id) do assign(conn, :current_message, message) else _ -> ChatApiWeb.FallbackController.call(conn, {:error, :not_found}) |> halt() @@ -92,11 +92,8 @@ defmodule ChatApiWeb.MessageController do @spec create(Plug.Conn.t(), map()) :: Plug.Conn.t() def create(conn, %{"message" => message_params}) do - with %{id: user_id, account_id: account_id} <- conn.assigns.current_user, - {:ok, %Message{} = msg} <- - message_params - |> Map.merge(%{"user_id" => user_id, "account_id" => account_id}) - |> Messages.create_message(), + with {:ok, params} <- sanitize_new_message_params(conn, message_params), + {:ok, %Message{} = msg} <- Messages.create_message(params), message <- Messages.get_message!(msg.id) do broadcast_new_message(message) @@ -146,6 +143,41 @@ defmodule ChatApiWeb.MessageController do end end + @spec sanitize_new_message_params(Plug.Conn.t(), map()) :: + {:ok, map()} | {:error, atom(), binary()} + defp sanitize_new_message_params( + _conn, + %{"customer_id" => customer_id, "user_id" => user_id} + ) + when is_binary(customer_id) and not is_nil(user_id) do + {:error, :unprocessable_entity, + "A message should not have both a `user_id` and `customer_id`"} + end + + defp sanitize_new_message_params(conn, %{"customer_id" => customer_id} = params) + when is_binary(customer_id) do + account_id = conn.assigns.current_user.account_id + customer = ChatApi.Customers.get_customer!(customer_id, []) + + case customer do + %{account_id: ^account_id} -> + {:ok, Map.merge(params, %{"account_id" => account_id})} + + _ -> + {:error, :forbidden, "Forbidden: invalid `customer_id`"} + end + end + + defp sanitize_new_message_params(conn, params) do + case conn.assigns.current_user do + %{id: user_id, account_id: account_id} -> + {:ok, Map.merge(params, %{"user_id" => user_id, "account_id" => account_id})} + + _ -> + {:error, :unauthorized, "Access denied"} + end + end + defp broadcast_new_message(message) do message |> Messages.Notification.broadcast_to_customer!() diff --git a/test/chat_api_web/controllers/message_controller_test.exs b/test/chat_api_web/controllers/message_controller_test.exs index e911e0052..ec892eb73 100644 --- a/test/chat_api_web/controllers/message_controller_test.exs +++ b/test/chat_api_web/controllers/message_controller_test.exs @@ -24,7 +24,8 @@ defmodule ChatApiWeb.MessageControllerTest do authed_conn: authed_conn, account: account, message: message, - conversation: conversation} + conversation: conversation, + user: user} end describe "index" do @@ -59,13 +60,131 @@ defmodule ChatApiWeb.MessageControllerTest do assert %{"id" => id} = json_response(conn, 201)["data"] conn = get(authed_conn, Routes.message_path(authed_conn, :show, id)) + user_id = authed_conn.assigns.current_user.id + account_id = authed_conn.assigns.current_user.account_id assert %{ "id" => _id, "object" => "message", - "body" => "some body" + "body" => "some body", + "user_id" => ^user_id, + "account_id" => ^account_id } = json_response(conn, 200)["data"] end + + test "defaults to the authed user's id and account_id when none are specified", + %{ + authed_conn: authed_conn, + conversation: conversation + } do + message = %{ + body: "some body", + conversation_id: conversation.id + } + + conn = post(authed_conn, Routes.message_path(authed_conn, :create), message: message) + assert %{"id" => id} = json_response(conn, 201)["data"] + + conn = get(authed_conn, Routes.message_path(authed_conn, :show, id)) + user_id = authed_conn.assigns.current_user.id + account_id = authed_conn.assigns.current_user.account_id + + assert %{ + "id" => _id, + "object" => "message", + "body" => "some body", + "user_id" => ^user_id, + "account_id" => ^account_id + } = json_response(conn, 200)["data"] + end + + test "renders message when customer ID is valid", + %{ + authed_conn: authed_conn, + account: account, + conversation: conversation + } do + customer = insert(:customer, account: account) + + message = %{ + body: "some body", + account_id: account.id, + customer_id: customer.id, + conversation_id: conversation.id + } + + conn = post(authed_conn, Routes.message_path(authed_conn, :create), message: message) + assert %{"id" => id} = json_response(conn, 201)["data"] + + conn = get(authed_conn, Routes.message_path(authed_conn, :show, id)) + customer_id = customer.id + account_id = authed_conn.assigns.current_user.account_id + + assert %{ + "id" => _id, + "object" => "message", + "body" => "some body", + "customer_id" => ^customer_id, + "account_id" => ^account_id + } = json_response(conn, 200)["data"] + end + + test "returns error when customer ID is not associated with the authenticated account", + %{ + authed_conn: authed_conn, + conversation: conversation + } do + some_other_account = insert(:account) + customer = insert(:customer, account: some_other_account) + + message = %{ + body: "some body", + customer_id: customer.id, + conversation_id: conversation.id + } + + conn = post(authed_conn, Routes.message_path(authed_conn, :create), message: message) + assert json_response(conn, 403)["error"]["message"] + end + + test "returns error when both a user ID and customer ID are specified", + %{ + authed_conn: authed_conn, + account: account, + conversation: conversation + } do + customer = insert(:customer, account: account) + + message = %{ + body: "some body", + account_id: account.id, + customer_id: customer.id, + user_id: authed_conn.assigns.current_user.id, + conversation_id: conversation.id + } + + conn = post(authed_conn, Routes.message_path(authed_conn, :create), message: message) + assert json_response(conn, 422)["error"]["message"] + end + + test "returns error when connection is not authorized", + %{ + conn: conn, + account: account, + conversation: conversation + } do + customer = insert(:customer, account: account) + + message = %{ + body: "some body", + account_id: account.id, + customer_id: customer.id, + conversation_id: conversation.id + } + + conn = post(conn, Routes.message_path(conn, :create), message: message) + assert json_response(conn, 401)["error"]["message"] + end end describe "update message" do