Index action should be more customizable

[mgwidmann]

Currently the index action must specify something like:

def can?(%User{}, :index, Post), do: true

Which means the user object can access the post object. But theres no way to specify that the user should only be able to access their own posts. Ideally, if the index action looped through the result set and ran can?/3 on each item, it would be simpler and more flexible to configure. I'm proposing something like this instead:

def can?(%User{id: id}, action, %Post{user_id: id}) when action in [:index, :show], do: true

To clearly say a user may access their own post for the :index and :show actions.

[cpjk]

This is a good feature, but I'm not sure about the best way to implement it. What concerns me is looping over all the records for a table when we only want to index those records belonging to a particular user. My first thought is to check if a user can index their own posts (assuming that the records in question have a user_id attribute), and if so, fetch only their posts from the database. That way we aren't looping over all the posts every time. However, this solution will only work for records belonging to a user, so we would need something like 'plug authorize_resource through: :blog' if, for example, we want to index the posts belonging to a blog.

[mgwidmann]

If you use pattern matching with Canada and can?/3, its going to have to be checking access after pulling data back, not within the SQL query, which doesn't work for index routes.

Only other way to do it then is to build your own DSL which will allow you at runtime to formulate the correct Ecto query. Similar to CanCan, can :read, Post, user_id: user.id. It'd be annoying to have to specify this twice (once in Canada form and again so you can build the right ecto query) so you could take that data and then generate the can?/3 function for the user behind the scenes.

[jhosteny]

What about a protocol for a bulk load query, that would just be used by fetch_all?

For example:

defprotocol Canary.BulkLoad
  def query(subject)
end

defimpl Canary.BulkLoad, for: Post do
  def query(%User{role: :admin}) do
    from p in Post
  end

  def query(%User{id: user_id}) do
    from p in Post, where: p.id == ^user_id
  end
end

You could probably implement a default implementation of the protocol that did something simple, like the latter function above.

[mgwidmann]

Well, it'd be where: p.user_id == ^user_id but I get your point...

Im curious how that would tie into the Canada protocol. Would that mean that you'd need to specify the above plus something like:

defimpl Canada.Can, for: Post do
  def can?(%User{id: user_id}, :show, %Post{user_id: user_id}), do: true
  def can?(%User{}, :show, %Post{}), do: false
end

If we have to write out the same rule twice, thats not a great design to me...

[mgwidmann]

Maybe consider making a macro which writes both of them out?

[cpjk]

@jhosteny I really like the idea of a protocol, because it is elegant, easy to implement, and allows the user to clearly define the behaviour.

However, it adds another thing that the user needs to specify.

As @mgwidmann mentioned, a macro to construct the Canada.Can implementation would take burden off the user and provide behaviour closer to CanCan, but I think we need to think carefully before starting to add macros.

This is definitely one of the more important features, and I think it's important that we do this right.

[jhosteny]

One possibility is to use the suggested protocol for loading a single resource as well.

Of course, this would mean you would lose the ability to distinguish whether a resource exists or access to it is unauthorized for the single resource case.

Also, this moves the security check strictly into the query logic. I can't think of any offhand, but there may be cases where this is undesirable.

Otherwise, I think @mgwidmann is correct. There has to be an automatic way to translate a single specification of the authorization rule into a query.

On Wednesday, June 17, 2015, Chris Kelly notifications@github.com wrote:

@jhosteny https://github.com/jhosteny I really like the idea of a protocol, because it is elegant, easy to implement, and allows the user to clearly define the behaviour.

However, it adds another thing that the user needs to specify.

As @mgwidmann https://github.com/mgwidmann mentioned, a macro to construct the Canada.Can implementation would take burden off the user and provide behaviour closer to CanCan, but I think we need to think carefully before starting to add macros.

This is definitely one of the more important features, and I think it's important that we do this right.

— Reply to this email directly or view it on GitHub https://github.com/cpjk/canary/issues/15#issuecomment-112969342.

[mgwidmann]

I was thinking about this... The best thing to do is always utilize the database, drop Canada.

This gives the following benefits:

• Will always use the same database index for both show & index actions
• Show actions will trigger the same 404 error handling when a document is unauthorized (I always feel its best to hide unauthorized items and treat it as it doesnt exist)
• Simpler for the user to configure, just specify the query scope: &Post.by_user/2 increasing reusability and reliability

There may be other benefits I can't think of, but while the pattern matching implementation is an interesting and cool feature, I think that its best to rely on the database for this project. Maybe consider moving Canada to a secondary feature (and possibly dropping it later).

[cpjk]

Taking from both of your suggestions, I think that a nice way to do this would be to have both Canada (or a similar, custom protocol) and BulkLoad (or whatever it ends up being called) under the hood, and provide a DSL to allow the user to do something like:

%User{id: user_id} |> can(:read, %Post{user_id: user_id})
%User{id: user_id} |> cannot(:read, %Post{user_id: other_user_id}) 

which would compile to corresponding protocol implementations like so:

defimpl Canada.Can, for: User do
  def can?(%User{id: user_id}, :read, %Post{user_id: id}), do: true
  def can?(%User{id: user_id}, :read, %Post{user_id: other_user_id}), do: false
end

defimpl Canary.Bulkload, for: Post, do
  # corresponding BulkLoad implementations
end

By default, BulkLoad would load only the resources that the current user is authorized to load, unless specified otherwise in the DSL. This is how CanCan does it, which I think is a reasonable behaviour.

So, load_and_authorize_resource would call BulkLoad.query.

load_resource would, by default, still load all the resources, unless specified otherwise in the plug.

authorize resource would return true for the index action only if the user can index all the records for the given model.

The only plug whose behaviour would really change at first with this change is load_and_authorize_resource

[mgwidmann]

I'm guessing that this example could also be written like this:

%User{id: user_id} |> can(:read, %Post{user_id: user_id})
 # no point in writing out the other user id since above should take care of it as an "else" condition
%User{id: user_id} |> cannot(:read, %Post{})

And it would work the same way? Thats how pattern matching would handle it, so I'd assume that would also work.

It all depends if you think you can make a can/3 and cannot/3 macro to output the code you put up.

Where would that above code go? You won't want to make the can/3/cannot/3 macro used globally, so you will want to have it in some module.

defmodule User do
  use Ecto.Model
  use Canary.Attributes

  schema "users" do
    #...
  end

  # Will `can` be called here?
end

# Will `can` be called here?

# Or in some other module like: (just making stuff up here...)
defmodule User.Attributes do
  use Canary.Attributes, for: User

  # will `can` be called here?
end

I guess it boils down to this problem: We want to use macros to provide defimpl definitions where defimpl is typically defined outside modules (unsure if thats a requirement) and using a macro means we must then pollute the global space with use Something.With.Macros globally which is likely to be labeled as an anti-pattern by many elixir users.

[mgwidmann]

After thinking about it for a moment, I think I like this solution:

defmodule User.Attributes do
  use Canary.Attributes, for: User

  for action <- [:create, :read, :update, :delete] do
    %User{id: user_id} |> can(action, %Post{user_id: user_id})
    %User{admin: true} |> can(action, %Post{})
    # May want to consider adding a default false case like this automatically in a before_compile
    %User{} |> cannot(action, %Post{})
  end

  # Custom actions
  %User{admin: true} |> can(:approve, %Post{})
end

The bonus to this is it could be written directly into the User model or as a nested module or even as a completely separate file.

[cpjk]

I was thinking a separate module, like in an abilities.ex:

defmodule Abilities do
  use Canary.Abilities

  %User{id: user_id} |> can(:read, %Post{user_id: user_id})
end

Given that the code generated by the DSL will generate namespaced protocol implementations, i.e. Canada.can?/3 and Canary.BulkLoad.query/* , why do you think it will pollute the global namespace?

[mgwidmann]

Nevermind...

I typically do my defimpl like this (as I've seen others):

defmodule SomeModule do
  # ...
end

defimpl Some.Protocol, for: SomeModule do
  # ...
end

But I tried out moving the defimpl inside the module and it seemed to work just fine, so all is well.

[mgwidmann]

was there any progress on this issue?

[cpjk]

Hey, I looked into an implementation, but it ended up being messy. I think that if you want to have more customizability, rolling your own plug auth (possibly on top of Canada) is probably the best bet. I should have closed this issue a long time ago.