Built my first simple controller: Need someone to check my code, offer advice

Phoenix Forum Questions / Help
1

I successfully scrapped together a simple API controller that accepts a JSON POST request and is supposed to return a token. However, I’m not confident that it is coded correctly, or built efficiently. This is my first real attempt at Elixir/Phoenix coding.

A valid request parameter will look like this:

%{
  "user" => %{
    "id" => "1",
    "username" => "Mike",
    "secret_key" => "secret_value_123"
  }
}

If you read the @doc notes, you can get an idea of what I’m trying to do.

One thing I could not figure out or find online is how to delete the “secret_key” parameter from the “user” map before putting it into the token generator. Instead, I had to manually rebuild the entire “user” map, which seems pretty messy, although it appears to have worked.

defmodule ExchatWeb.UserController do
  use Phoenix.Controller

  @doc """
  This controller receives a PHP curl POST request that includes the userid, 
username and a secret variable, and sends back a token if everything checks out.

  Step 1) Runs the function if all those parameters exist in the user object. 
Otherwise runs the second "catch-all" error function that sends an empty 
JSON array back.

  Step 2) Runs a conditional check on the "secret_key" variable. This secret 
variable (name and value) is hardcoded in the PHP script, so the client or 
XSS/AJAX attackers should never know about it in case they learn of the 
token endpoint location, hence they cannot get a token without knowing 
the "secret" variable. Only my PHP script can make the calls to Phoenix
 to create tokens. If "secret_key" is not found or does not equal 
"secret_value_123", send empty JSON array back. Security Note: 
Apache is configured to block all requests, except from the server IP 
where only my websites exist - to prevent external attacks.

  Step 3) If "secret" checks out, delete "secret_key" variable from the 
user object before storing it as a token (to prevent client from ever 
viewing "secret" inside the token). Send token back.
  """

  def login(conn, %{"user" => %{"id" => userid, "username" => username, "secret_key" => secret}} = user) do
    IO.inspect(user)
    cond do
      secret in ["secret_value_123"] ->
        user = %{"user" => %{"id" => userid, "username" => username}}
        token = Phoenix.Token.sign(ExchatWeb.Endpoint, "user_auth", user)
        IO.inspect(user)
        json(conn, %{token: token})
      true ->
        json(conn, %{})
    end
  end

  # catch-all error: sends back an empty JSON array for all requests that don't have required params
  def login(conn, %{}) do
    json(conn, %{})
  end
end

I was hoping an experienced member here could re-write it the “correct way” or a much more efficient way. I can then study it and learn. Any advice is greatly appreciated.

2

Without speaking much to the correctness of your business case, there are a couple of idiomatic improvements you can do to the code.

  1. Pattern matching
    Since you are hard-coding the "secret_key" => "secret_value_123", you can pattern match it directly on the function signature such that anything else falls through to your catch-all definition. As a bonus, if your secret key is some large random string you can also store it separately as a module constant or eventually as an application configuration parameter in e.g. config.ex.
@secret_key "secret_value_123"
def login(conn, %{"user" => %{"id" => _, "username" => _, "secret_key" => @secret_key}} = user) do
  1. Map manipulation
    As you’ve said, rebuilding the map can get messy for larger maps, which end up using more memory than necessary since you’re making new strings and structures.
    You can leverage Kernel.update_in/2 to reach into your user map and update the internal "user" value, passing it an update function that uses Map.take/2.
user = update_in(user["user"], &Map.take(&1, ["id", "username"]))

A word of warning for this use case is that it’s blindly keeping the outer map part.
When pattern-matching on the function, any other key in the original user param would also be preserved. e.g. %{"user" => _, "malicious_data" => _a_really_large_string_or_map}

Depending on how you’re using the token elsewhere you could just encode the inner map

token = Phoenix.Token.sign(ExchatWeb.Endpoint, "user_auth", Map.take(user["user"], ["id", "username"])

that is just store %{"id" => "1", "username" => "Mike"}.

Or event better, pull out the inner user data in your function declaration and automatically ignore extra keys in your params.

The end result would be:

defmodule ExchatWeb.UserController do
  use Phoenix.Controller

  @secret_key "secret_value_123"

  def login(conn, %{"user" => %{"id" => _, "username" => _, "secret_key" => @secret_key} = inner_user }) do
        user = Map.take(inner_user, ["id", "username"])

        token = Phoenix.Token.sign(ExchatWeb.Endpoint, "user_auth", user)

        json(conn, %{token: token})
  end

  # catch-all error: sends back an empty JSON array for all requests that don't have required params
  def login(conn, %{}) do
    json(conn, %{})
  end
end
5 Likes
3

Thanks a lot for the insight, I greatly appreciate it and it taught me a few new things!

1 Like