cpjk / canary

:hatching_chick: Elixir authorization and resource-loading library for Plug applications.
MIT License
474 stars 51 forks source link

nil comparison forbidden #71

Open philippneugebauer opened 7 years ago

philippneugebauer commented 7 years ago

When I want to access one of my custom routes, I get the following error

nil given for :id. Comparison with nil is forbidden as it is unsafe.
Instead write a query with is_nil/1, for example: is_nil(s.id)

That's probably the interesting part of the call log:

 lib/canary/plugs.ex:296 Canary.Plugs.fetch_resource/2
 lib/canary/plugs.ex:112 Canary.Plugs.do_load_resource/2
 lib/canary/plugs.ex:92 Canary.Plugs.load_resource/2
 lib/canary/plugs.ex:271 Canary.Plugs.do_load_and_authorize_resource

I thought that was solved by non_id_actions but it does not help.

philippneugebauer commented 7 years ago

I found the problematic function and line:

  defp do_load_resource(conn, opts) do
    ...

    loaded_resource =
      cond do
        is_persisted ->
          fetch_resource(conn, opts)
        action == :index ->
          fetch_all(conn, opts)
        action in [:new, :create] ->
          nil
        true ->
          fetch_resource(conn, opts)
      end

    ...
  end

action in [:new, :create] -> does not regard other non-id-actions while do_authorize_resource does:

  defp do_authorize_resource(conn, opts) do
    ...
    non_id_actions =
      if opts[:non_id_actions] do
        Enum.concat([:index, :new, :create], opts[:non_id_actions])
      else
        [:index, :new, :create]
      end

I refactored do_load_resourcea bit according to do_authorize_resource resulting in the following and now it works:

    create_actions =
      if opts[:non_id_actions] do
        Enum.concat([:new, :create], opts[:non_id_actions])
      else
        [:new, :create]
      end

    loaded_resource =
      cond do
        is_persisted ->
          fetch_resource(conn, opts)
        action == :index ->
          fetch_all(conn, opts)
        action in create_actions ->
          nil
        true ->
          fetch_resource(conn, opts)
      end

Let me know what you think about that and if it's fine, I will do a PR