cpjk / canary

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

nil given for :id #50

Closed rlopzc closed 7 years ago

rlopzc commented 7 years ago

In my page controller

plug :authorize_resource, model: User

In my router

scope "/admin", as: :admin, alias: Soranus do
    pipe_through [:browser, :browser_session, :browser_auth]
    get "/", PageController, :admin_index
  end

My test

describe "logged in admin" do
    setup do
      user =
        params_for(:user)
        |> user_with_registration_changeset()
      conn =
        guardian_login(build_conn(), user)

      {:ok, conn: conn, user: user}
    end

    test "get admin index when user_type = :admin", %{conn: conn} do
      conn = get conn, admin_page_path(conn, :admin_index)
      assert html_response(conn, 200)
    end
  end

The function guardian_login sets the user in conn, and in my pipline browser_auth i use a Plug to set the current_user to conn from Guardian.Plug.current_resource(conn) Like this

defmodule Soranus.Authentication.PlugCurrentUser do
  def init(opts), do: opts

  @lint {Credo.Check.Design.AliasUsage, false}
  def call(conn, _opts) do
    current_user = Guardian.Plug.current_resource(conn)
    Plug.Conn.assign(conn, :current_user, current_user)
  end
end

Error in conn = get conn, admin_page_path(conn, :admin_index) in my tests (above)

Error:

2) test logged in admin get admin index when user_type = :admin (Soranus.PageControllerTest)
     test/controllers/page_controller_test.exs:25
     ** (ArgumentError) 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)
     stacktrace:
       (ecto) lib/ecto/query/builder/filter.ex:107: Ecto.Query.Builder.Filter.runtime!/6
       (ecto) lib/ecto/query/builder/filter.ex:98: Ecto.Query.Builder.Filter.runtime!/2
       (ecto) lib/ecto/repo/queryable.ex:304: Ecto.Repo.Queryable.query_for_get_by/3
       (ecto) lib/ecto/repo/queryable.ex:52: Ecto.Repo.Queryable.get_by/5
       lib/canary/plugs.ex:298: Canary.Plugs.fetch_resource/2
       lib/canary/plugs.ex:203: Canary.Plugs._authorize_resource/2
       lib/canary/plugs.ex:186: Canary.Plugs.authorize_resource/2
       (soranus) web/controllers/page_controller.ex:1: Soranus.PageController.phoenix_controller_pipeline/2
       (soranus) lib/soranus/endpoint.ex:1: Soranus.Endpoint.instrument/4
       (soranus) lib/phoenix/router.ex:261: Soranus.Router.dispatch/2
       (soranus) web/router.ex:1: Soranus.Router.do_call/2
       (soranus) lib/soranus/endpoint.ex:1: Soranus.Endpoint.phoenix_pipeline/1
       (soranus) lib/soranus/endpoint.ex:1: Soranus.Endpoint.call/2
       (phoenix) lib/phoenix/test/conn_test.ex:224: Phoenix.ConnTest.dispatch/5
       test/controllers/page_controller_test.exs:26: (test)
cpjk commented 7 years ago

@romariolopez have you tried inspecting the state of the conn at that point? (You can do this by putting an IEx.pry binding before line 298 in deps/canary/lib/canary/plugs.ex).

rlopzc commented 7 years ago

@cpjk Sorry, i've been busy. I just inspected conn and it appears that current_user is loaded, i'm even using canary in another controller and is working fine, I want to use canary for an :admin_index, and the model that i'm passing is Soranus.User, i don't know if there are any inconveniences.

%Plug.Conn{adapter: {Plug.Adapters.Cowboy.Conn, :...},
 assigns: %{current_user: %Soranus.User{__meta__: #Ecto.Schema.Metadata<:loaded, "users">,
    email: "admin@gmail.com", id: 1,
    inserted_at: #Ecto.DateTime<2016-09-19 17:43:24>,
    medical_informations: #Ecto.Association.NotLoaded<association :medical_informations is not loaded>,
    password: nil,
    password_hash: "$2b$12$Oiekzoz/el5fAktWQ9XgguHs2DMfWYIqAAvqh4aVDYsor9AoNnIi.",
    updated_at: #Ecto.DateTime<2016-09-19 17:43:24>, user_type: :admin}},
 before_send: [#Function<0.7834419/1 in Plug.CSRFProtection.call/2>,
  #Function<4.55932481/1 in Phoenix.Controller.fetch_flash/2>,
  #Function<0.82590416/1 in Plug.Session.before_send/2>,
  #Function<1.73933339/1 in Plug.Logger.call/2>,
  #Function<0.131639940/1 in Phoenix.LiveReloader.before_send_inject_reloader/1>],
 body_params: %{},
...

In my case = :error at line 297 get_map_args = %{id: nil} and this is the error, my params = %{}, but i don't know why canary expects an id in params, i'm just using authorize_resource/2, why does canary expects an id?, i'm trying to authorize an index with this canada config

  def can?(%User{id: user_id, user_type: :admin}, _action, User), do: true
rlopzc commented 7 years ago

I found the error, my action is :admin_index but canary is expecting the action names

action in [:index, :new, :create] ->
        opts[:model]

at line 199, i thought that i could authorize any action without using a persisted model.

How do i authorize a user to execute any action (different from :index, :new, :create without a related PERSISTED model? For example, :admin_index, :sales_report, and those actions are only visible for the :admin user. @cpjk

rlopzc commented 7 years ago

This is solved by #45