How to properly send and manage 403 responses

I do my permission testing, which if that fails it throws match error of a special tagged tuple, which I catch in my generic error catcher here:

# ... snip others
  def handle_error({:perm, false}, conn)              ,do: do_unauthorized(conn)
# ... snip others

Where do_unauthorized/1 is:

  def do_unauthorized(conn) do
    ControllerCallbacks.unauthorized(conn)
  end

Where unauthorized/1 is:

  def unauthorized(conn, _params \\ %{}) do
    last_path = case conn.method do
      "GET" -> true
      _ -> false
    end |> case do
      false -> nil
      true -> conn.request_path <> if byte_size(conn.query_string) > 0 do "?" <> conn.query_string else "" end
    end

    conn
    |> put_session(:last_path, last_path)
    |> put_flash(:error, gettext("The current account does not have acces to that location, please log in with an account that does or contact an admin to add access if it is required."))
    |> redirect(to: auth_path(conn, :ldap_request))
  end

It just gets the location of where they were, the query, stories it, and redirects to the auth controller to have them log in with a better account (which immediately clears the stored path from the conn but stores it in another place internally and will redirect to it if they successfully log in with another account, among some other stuff). This unauthorized/1 function could easily just set a 403 status and render a template though.

Is it a good practice to call ControllerCallbacks.unauthorized(conn) from the plug? Or maybe unauthorized/1 could be inside the plug?

This is the plug:

defmodule App.Auth do
  import Plug.Conn

  def init(opts) do
    opts
  end

  def call(conn, _opts) do
    conn.assigns[:current_user]
    |> is_admin
    |> allow_superusers(conn)
  end

  defp is_admin(%{is_superuser: true}) do
    true
  end

  defp is_admin(_) do
    false
  end

  defp allow_superusers(true, conn) do
    conn
  end

  defp allow_superusers(_, _conn) do
    raise App.UnauthorizedError

    # This is an alternative
    # conn
    # |> put_status(403) 
    # |> send_resp(403, "Unauthorized")
    # |> halt
  end

end

I like to have everything that mutates a connection in a controller somewhere, or the helpers for my shared bits, absolutely unnecessary though, just an organizational thing for me.

EDIT: I would not really raise the error there, I have a wrapper around all my leaf-node controllers to catch my custom errors.

So, If I understand correctly, you propose to alter the connection in a controller module calling the function in the plug like:

defp allow_superusers(_, _conn) do
  OtherModule.unauthorized(conn)
end

what do you mean with having a wrapper around all the leaf-node controllers? I want to avoid using custom errors, so I really like the idea you proposed of using a function like this:

def unauthorized(conn, _params \\ %{}) do
    last_path = case conn.method do
      "GET" -> true
      _ -> false
    end |> case do
      false -> nil
      true -> conn.request_path <> if byte_size(conn.query_string) > 0 do "?" <> conn.query_string else "" end
    end

    conn
    |> put_session(:last_path, last_path)
    |> put_flash(:error, gettext("The current account does not have acces to that location, please log in with an account that does or contact an admin to add access if it is required."))
    |> redirect(to: auth_path(conn, :ldap_request))
  end

My controller commands are like this (this is before a refactor in rearranging things, but still):

  def index_section(conn, %{"slug" => slug_param}) do
    happy_path!(else: handle_error(conn)) do
      @perm true = conn |> can?(show(%Perms.CheckInOut{section: true}))
      {:ok, section} = verify_single_record get_section_by_slug(slug_param)
      @perm true = conn |> can?(show(%Perms.CheckInOut{section: section.slug}))
      section = preload_section_with_checked_out_values(section)
      changeset = %CheckInOut.Value{section_id: section.id}
      |> CheckInOut.Value.changeset()
      render(conn, :index_section, section: section, changeset: changeset)
    end
  end

The happy_path!(else: handle_error(conn)) do handles the errors. The lines @perm true = conn |> can?(show(%Perms.CheckInOut{section: true})) and @perm true = conn |> can?(show(%Perms.CheckInOut{section: section.slug})) test permissions and return true if successful, so if that fails then it fails the match to true, and thus gets tagged with a :perm tuple via the @perm tag, which gets tossed into the handle_error(conn) call, which enters the path you saw before. I have very fine-grained permissions that are built up based on data that gets accessed (with a lot of early tests, like here it tests if they have access to ‘anything’ in the section, if true then it gets the section information and tests if they have access to this specific section, if true then it continues). This is why a Plug style permission tests would not even remotely work for me, they are far too coarse-grained, at least without multiple DB lookups, which…why, or stuffing the conn assign with lots of stuff?

Very useful, thank you!!

As an aside: for the benefit of fellow programmers and your future self, you should consider renaming your errors and functions from “Unauthorized” to “Forbidden”, as the former suggests (at least to me) a 401 error rather than 403.

Let me join the fun

I was the one who suggested @adrian to use custom errors on the #phoenix-talk google group (as a fellow beginner). I personally used the method when implementing parameter validation for my routes (using GitHub - vic/params: Easy parameters validation/casting with Ecto.Schema, akin to Rails' strong parameters. · GitHub, raising InvalidParamsError on not ch.valid?). My justification is that I wanted to treat it as 422, and the Phoenix docs seem to suggest using custom errors for that (as they have been doing with Ecto.NoResultsError and others).

That way, my controller can be as simple as

defparams media_index_params %{
  limit: [field: :integer, default: 10],
  offset: [field: :integer, default: 0],
}

def index(conn, params) do
  validated_params = ParamsValidator.validate params, &media_index_params/1
  media = Media.fetch(validated_params)
  render(conn, "index.json", media: media)
end

The ParamsValidator.validate/1 function is the one that will raise the error. This is just a small API project and I don’t handle authentication and permission at all. I see @OvermindDL1’s solution as kind of convoluted but maybe it’s because I just haven’t worked on something of that scale yet (or because I haven’t seen the bigger picture ).

I do want feedbacks if there is a more recommended approach than using custom errors, or if there are drawbacks by using it.

True true, unauthorized should be 401 if you are returning codes.[quote=“bobbypriambodo, post:9, topic:2191”]
The ParamsValidator.validate/1 function is the one that will raise the error. This is just a small API project and I don’t handle authentication and permission at all. I see @OvermindDL1’s solution as kind of convoluted but maybe it’s because I just haven’t worked on something of that scale yet (or because I haven’t seen the bigger picture ).
[/quote]

It could very well be convoluted! ^.^
It grew this way because the error handling has become massive and can come from a variety of parts, like on my example above the verify_single_record call will cause a Match Error on the {:ok, section} if there is not a single record (I.E., it is not found), the special error tag it returns it matched for in my error handlers to return an appropriate NotFound code or so:

  def handle_error({:error, :no_record}, conn)        ,do: do_not_found(conn)
  def handle_error({:error, :too_many_records}, conn) ,do: do_unprocessable_entity(conn)

I have a long list here of various generic things that can be returned from controllers or functions they call. The not found and unprocessable entities, like most, are more generic than my unauthorized (which redirects to let them login better):

  def do_not_found(conn) do
    conn
    |> put_status(:not_found)
    |> render(MyServer.ErrorView, :not_found)
  end

  def do_unprocessable_entity(conn) do
    conn
    |> put_status(:unprocessable_entity)
    |> render(MyServer.ErrorView, :unprocessable_entity)
  end

And is probably more of what the OP wants to do.