Introduces authorize_controller functionality #61
Conversation
send calling controller to the canada.can? function instead of model
lib/canary/plugs.ex
Outdated
| @@ -181,6 +181,27 @@ defmodule Canary.Plugs do | |||
| plug :load_resource, model: Post, id_name: "slug", id_field: "slug", only: [:show], persisted: true | |||
| ``` | |||
| """ | |||
| def authorize_controller(%{assigns: %{current_user: nil}} = conn, opts) do | |||
There was a problem hiding this comment.
Why %{current_user: nil}? It looks like this makes the assumption that a nil user should always be unauthorized. However, this is not always true. For example, we might not assign a current_user for users that are not logged in, but we still want non-logged in users to be able to access certain controller actions.
There was a problem hiding this comment.
okay I got it, it was my bad understanding of the tests. will fix this also
lib/canary/plugs.ex
Outdated
| conn | ||
| end | ||
| end | ||
| def authorize_controller(conn, _), do: Plug.Conn.assign(conn, :authorized, true) |
There was a problem hiding this comment.
It looks like this is saying "whenever there is no phoenix_controller set in the conn, set authorized: true"
I think it would be better to delete this line, and allow the match error to be raised. If someone is using authorize_controller, they should know to set phoenix_controller in the conn, and it makes sense that an error should occur if it is not there.
There was a problem hiding this comment.
yes this is rather excessive. again it was my bad understanding of the test
There was a problem hiding this comment.
did you have time to check the changes ? are they okay now ?
lib/canary/plugs.ex
Outdated
| |> handle_unauthorized(opts) | ||
| end | ||
| def authorize_controller(%{assigns: %{current_user: user}, | ||
| private: %{phoenix_controller: controller}} = conn, opts) do |
There was a problem hiding this comment.
I think it would be nice to allow non-phoenix users to use a controller key other than phoenix_controller. Maybe canary_controller?
So we would fetch the controller like controller = conn.assigns[:canary_controller] || conn.assigns[:phoenix_controller] or something like that.
There was a problem hiding this comment.
agree, didn't think about this point at all.
1 - does not authomatically deauthenticate when current_user is nil 2 - allow allow non-phoenix users to use a controller key using :canary_controller 3 - when the controller is not set properly fail the authentication 4 - updates the tests accordingly
lib/canary/plugs.ex
Outdated
| @@ -181,6 +181,22 @@ defmodule Canary.Plugs do | |||
| plug :load_resource, model: Post, id_name: "slug", id_field: "slug", only: [:show], persisted: true | |||
| ``` | |||
| """ | |||
| def authorize_controller(conn, opts) do | |||
There was a problem hiding this comment.
I'm sorry for missing this before, but we should move authorize_controller so that it is below do_authorize_resource. This will keep the @doc string above with authorize_resource, since that is the function it refers to. It will also keep do_authorize_resource with authorize_resource.
We should also add a new @doc string above authorize_controller, so that it will be added to the hex docs, and a section in the README.md describing the usage of authorize_controller.
There was a problem hiding this comment.
Done :)
Please check if the documentation is good enough.
Thanks a lot
There was a problem hiding this comment.
Just 2 more comments. Sorry for the repeated change requests, but it's better if we make sure everything is fine now, rather than having to fix bugs later :)
adds documentation
lib/canary/plugs.ex
Outdated
| 2) :canary_controller in conn.assigns | ||
| In case you are not using phoenix, make sure you set the controller name in the conn.assigns | ||
| Note that in case neither of :phoenix_controller or :canary_controller are found the requested | ||
| authorization will automatically fails |
There was a problem hiding this comment.
From controller = conn.assigns[:canary_controller] || conn.private[:phoenix_controller], it looks like when neither phoenix_controller or canary_controller are present, controller will be nil, meaning that we will just authorize against a nil controller in Canada, so authorization will not necessarily fail (this could be set to true in the Canada rules). I think this behaviour is fine since it is consistent with how authorize_resource works, but we should make sure the docs here are consistent with the code.
There was a problem hiding this comment.
Yes, you're right. I totally missed this. I have updated it
test/plug_test.exs
Outdated
| expected = Plug.Conn.assign(conn, :authorized, false) | ||
|
|
||
| assert authorize_controller(conn, opts) == expected | ||
| end |
There was a problem hiding this comment.
We should probably add a test for when authorization fails because a rule specified that the user could not access a given controller (as a regression test).
There was a problem hiding this comment.
I added two more test for a controller that has partial access
|
LGTM! Thanks for the PR!
|
|
My question is related to this PR. I don't find any |
send calling controller to the canada.can? function instead of model
This way a user can be authorized based on the action And the controller the action is called from