Controller style & structure in Phoenix 1.3

Questions / Help
1

I’ve been working on a Phoenix 1.3 app and would like your advice on whether the kind of code displayed below is going in the right direction or instead has major design flaws. In particular, is there work that should be offloaded to a plug?? I’m using a home-grown authentication system in which a registered user can present email and password and receive a JWT token which is needed to access all routes except /public/documents – and of course the one used to obtain the token.

I’d like to express my continuing thanks to all in this forum … your specific help and general advice is invaluable … couldn’t work without it

Two typical controller actions:

def show(conn, %{"id" => id}) do
    document = DocManager.get_document!(id)
   # {:ok, user_id} returned if token is valid and carries the user_id,
   # otherwise {:error, "note authorized" } is returned.
   # Users are allowed to read their own docs and public docs
    with {:ok, user_id} <- Token.user_id_from_header(conn),
         true <- ((document.attributes["public"] == true) || (user_id == document.author_id))
    do
      render(conn, "show.json", document: document)
    else
      {:error, error} -> {:error, error}
    end
  end

  def show_public(conn, %{"id" => id}) do
    document = DocManager.get_document!(id)
    if document.attributes["public"] == true do
      render(conn, "show.json", document: document)
    else
        {:error, "Cannot display document"}
    end
  end
2

Maybe you could use different routes for unauthenticated access to public documents vs. authenticated access to own documents. This way you can separate out which things you need to check. Also I’d only offload general authentication into a plug. As soon as you need the document to decide if the route is accessable or not it belongs into the controller in my opinion.

3

Sorry for the dumb question. But is it a new 1.3 feature that you can return an :error tuple? I never used that (and neither seen that in the docs).

Marcus

4

I believe so. The tuple is then handled by the fallback contoller, which centralizes all error handling. Here is one article on the subject and here is Chris McCord’s announcement

5

The action callback you could override earlier too, I’ve been overriding it for custom error handling for over a year now. :slight_smile:

6

A bit off-topic, sorry, but since with returns any unmatched expression anyway

iex(1)> with {:ok, "a"} <- {:error, "b"}, do: :ok # no need for `else` block
{:error, "b"}

your controller actions can be simplified a bit

def show(conn, %{"id" => id}) do
  document = DocManager.get_document!(id)
  # {:ok, user_id} returned if token is valid and carries the user_id,
  # otherwise {:error, "note authorized" } is returned.
  # Users are allowed to read their own docs and public docs
  with {:ok, user_id} <- Token.user_id_from_header(conn),
       true <- document.attributes["public"] or user_id == document.author_id,
       do: render(conn, "show.json", document: document)
end

def show_public(conn, %{"id" => id}) do
  document = DocManager.get_document!(id)
  if document.attributes["public"] do
    render(conn, "show.json", document: document)
  else
    {:error, "Cannot display document"}
  end
end

Assuming attributes is a map, I would also separate true <- document.attributes["public"] or user_id == document.author_id into helper functions for better errors like

defp public_or_author(%{attributes: %{"public" => true}})

defp public?(%{attributes: %{"public" => true}}, do: :yep
defp public?(%{attributes: %{"public" => false}}, do: {:error, :not_public}

defp author?(_id, %{author_id: _id}), do: :yep
defp author?(_id, %{author_id: _author_id}), do: {:error, :not_author}

so that the with block becomes

with {:ok, user_id} <- Token.user_id_from_header(conn),
     :yep <- public?(document),
     :yep <- author?(user_id, document),
     do: render(conn, "show.json", document: document)
2 Likes
7

Thankyou! Great suggestions.