From 593ec2d2be921b7af585150fe100ad48271f98fb Mon Sep 17 00:00:00 2001 From: reichert621 Date: Thu, 14 Jan 2021 09:33:07 -0500 Subject: [PATCH 1/2] Add some todo comments --- lib/chat_api/customers.ex | 2 ++ lib/chat_api_web/controllers/customer_controller.ex | 1 + 2 files changed, 3 insertions(+) diff --git a/lib/chat_api/customers.ex b/lib/chat_api/customers.ex index 9a46a78dd..639603893 100644 --- a/lib/chat_api/customers.ex +++ b/lib/chat_api/customers.ex @@ -44,6 +44,8 @@ defmodule ChatApi.Customers do @spec find_by_external_id(binary(), binary(), map()) :: Customer.t() | nil def find_by_external_id(external_id, account_id, filters \\ %{}) + # TODO: return list rather than just one, and then ignore situations where there are multiple with the same ID + # TODO: factor in IP address as well? eh, maybe not... but it might be better to just make this super restrictive? def find_by_external_id(external_id, account_id, filters) when is_binary(external_id) do Customer |> where(account_id: ^account_id, external_id: ^external_id) diff --git a/lib/chat_api_web/controllers/customer_controller.ex b/lib/chat_api_web/controllers/customer_controller.ex index 2f75c3f34..90548b6f3 100644 --- a/lib/chat_api_web/controllers/customer_controller.ex +++ b/lib/chat_api_web/controllers/customer_controller.ex @@ -58,6 +58,7 @@ defmodule ChatApiWeb.CustomerController do |> Enum.reject(fn {_k, v} -> blank?(v) end) |> Map.new() + # TODO: require `external_id` to be a minimum length/randomness? case Customers.find_by_external_id(external_id, account_id, filters) do %{id: customer_id} -> json(conn, %{ From af9f14152de213528b4ec20646c62518aaaa1109 Mon Sep 17 00:00:00 2001 From: reichert621 Date: Thu, 14 Jan 2021 15:07:07 -0500 Subject: [PATCH 2/2] Play around with some more potential improvements to identifying customers by external_id (WIP) --- lib/chat_api/customers.ex | 25 +++++++++ .../controllers/customer_controller.ex | 51 ++++++++++--------- 2 files changed, 51 insertions(+), 25 deletions(-) diff --git a/lib/chat_api/customers.ex b/lib/chat_api/customers.ex index 639603893..1cc9c6aa1 100644 --- a/lib/chat_api/customers.ex +++ b/lib/chat_api/customers.ex @@ -41,6 +41,22 @@ defmodule ChatApi.Customers do Customer |> Repo.get!(id) |> Repo.preload(:tags) end + @spec list_by_external_id(binary(), binary(), map()) :: [Customer.t()] + def list_by_external_id(external_id, account_id, filters \\ %{}) + + def list_by_external_id(external_id, account_id, filters) when is_binary(external_id) do + Customer + |> where(account_id: ^account_id, external_id: ^external_id) + |> where(^filter_where(filters)) + |> order_by(desc: :updated_at) + |> Repo.all() + end + + @spec list_by_external_id(integer(), binary(), map()) :: [Customer.t()] + def list_by_external_id(external_id, account_id, filters) when is_integer(external_id) do + external_id |> to_string() |> list_by_external_id(account_id, filters) + end + @spec find_by_external_id(binary(), binary(), map()) :: Customer.t() | nil def find_by_external_id(external_id, account_id, filters \\ %{}) @@ -252,6 +268,15 @@ defmodule ChatApi.Customers do |> Repo.delete() end + @spec is_secure_external_id?(any()) :: boolean() + def is_secure_external_id?(external_id) when is_binary(external_id) do + String.length(external_id) >= 8 && + !String.match?(external_id, ~r/^[[:alpha:]]+$/) && + !String.match?(external_id, ~r/^[[:digit:]]+$/) + end + + def is_secure_external_id?(_external_id), do: false + # Pulled from https://hexdocs.pm/ecto/dynamic-queries.html#building-dynamic-queries @spec filter_where(map) :: Ecto.Query.DynamicExpr.t() def filter_where(params) do diff --git a/lib/chat_api_web/controllers/customer_controller.ex b/lib/chat_api_web/controllers/customer_controller.ex index 90548b6f3..f5b750a67 100644 --- a/lib/chat_api_web/controllers/customer_controller.ex +++ b/lib/chat_api_web/controllers/customer_controller.ex @@ -6,7 +6,7 @@ defmodule ChatApiWeb.CustomerController do alias ChatApi.{Accounts, Customers} alias ChatApi.Customers.Customer - action_fallback ChatApiWeb.FallbackController + action_fallback(ChatApiWeb.FallbackController) @spec index(Plug.Conn.t(), map) :: Plug.Conn.t() def index(conn, params) do @@ -48,30 +48,31 @@ defmodule ChatApiWeb.CustomerController do "account_id" => account_id } = params ) do - # TODO: support whitelisting urls for an account so we only enable this and - # other chat widget-related APIs for incoming requests from supported urls? - if Accounts.exists?(account_id) do - # TODO: make "host" a required param? (but would have to ignore on mobile...) - filters = - params - |> Map.take(["email", "host"]) - |> Enum.reject(fn {_k, v} -> blank?(v) end) - |> Map.new() - - # TODO: require `external_id` to be a minimum length/randomness? - case Customers.find_by_external_id(external_id, account_id, filters) do - %{id: customer_id} -> - json(conn, %{ - data: %{ - customer_id: customer_id - } - }) - - _ -> - json(conn, %{data: %{customer_id: nil}}) - end - else - send_account_not_found_error(conn, account_id) + cond do + !Customers.is_secure_external_id?(external_id) -> + json(conn, %{data: %{customer_id: nil}}) + + Accounts.exists?(account_id) -> + filters = + params + |> Map.take(["email", "host"]) + |> Enum.reject(fn {_k, v} -> blank?(v) end) + |> Map.new() + + case Customers.list_by_external_id(external_id, account_id, filters) do + [%Customer{id: customer_id}] -> + json(conn, %{ + data: %{ + customer_id: customer_id + } + }) + + _ -> + json(conn, %{data: %{customer_id: nil}}) + end + + true -> + send_account_not_found_error(conn, account_id) end end