Custom authorizer and create? policy for LibraryEntry

[trmcnvn]

Defines a create? policy for LibraryEntry, this is a needed change as before the switch to jsonapi-authorizer we were calling create? after the fields had been replaced.

---

By default, jsonapi-authorizer calls update? on relationship objects when creating a resource. This fails for us in cases like creating a LibraryEntry where AnimePolicy#update? will always be false.

There is an active discussion about this upstream, so we may be able to get rid of this in the future.

[valscion]

Hi there! I'm one of the maintainers of jsonapi-authorization gem and I came here to see your issue.

When you check for create?, you should be checking only whether the current user is allowed to create any instances of the class under tests. That is because the authorization check should only be concerned with the question of "can I create a LibraryEntry?"

Now that brings us to the issue at hand, which is that there should be some way of authorizing for associations. That is exactly the same case as described in venuu/jsonapi-authorization#15 and which you have tried to overcome by simply not checking for the related records.

Am I right to assume that this looks like something you'd want to have in your LibraryEntryPolicy?

class LibraryEntryPolicy
  def associate_with_user?(new_user)
    # Only allow setting `library_entry.user` to the same as us for normal users
    new_user == user || is_admin?
  end
end

This is one of the possible solutions to venuu/jsonapi-authorization#15 but it feels a bit like going against the Pundit architecture. They would want to authorize on the backing instance for create? actions, too: https://github.com/elabs/pundit/issues/403

Admittedly, it would be OK to check for the backing instance if we could create one safely before we've at the point to call save. However, even this solution feels like it's a bit incorrect. If not incorrect, then at least it might be a bit difficult to achieve in the current implementation of jsonapi-authorization.

However, I wonder, is it really true that in your use case, you really want to be able to send the user_id in the request itself? I second @DNA's idea that automatically assigning user_id to the current_user actually seems like the proper option, as its kinda weird to be able to define any user_id when creating a LibraryEntry. Or am I completely mistaken with this? Do you really want to support the use case of assigning a LibraryEntry to a user other than the current one?

EDIT: Wow, I just took a good look into what you've built and what hummingbird.me really is and it's amazing! I'll be happy to help with your issues regarding authorization — I think that your app is a great test case for our gem in making gem sure that it can be ran for all use cases.

[NuckChorris]

Personally, I think it makes more sense to verify the user_id as the current_user than to implicitly set the user_id in the server.

If we set the user_id implicitly and the client didn't set a user_id, we (obviously) return that in the response to the creation request. But what happens when the client does set a user_id? To me, it makes more sense to throw an error than to implicitly overwrite it.

And then if we do set it, where do we do that? In the controller?

Authorizing on the instance does seem to be the approach advised by Pundit, and I can see other cases where it could be useful too (is the current user allowed to make a model with these specific attributes set? For example, genres or titles)

Or am I completely mistaken with this? Do you really want to support the use case of assigning a LibraryEntry to a user other than the current one?

There's some cases where this might make sense, for example if we want to allow admins to create things on behalf of users (LibraryEntry is probably not a reasonable place for this, but there are other places where it wouldn't be unreasonable)

Really it's more of shifting the logic from implicit server-side filling to explicit client-side filling, though.

[DNA]

Hi @valscion, thanks for the feedback! :)

Well, after giving more thought on the problem, it seems my initial idea wasn't so consistent as I hoped…

First problem is that current_user is a controller scope, so we can't have the model to define it automatically. Second, the site can import user data from another site, and those importer libs don't have a current_user set, but still need to define a valid user_id.

Now, back to the problem, since JR create controller callbacks related to the model callbacks, is it doable to attach authorization to them? I was thinking about something like that:

class LibraryEntryResource < BaseResource
  include JSONAPI::Authorization::PunditScopedResource

  authorize_callbacks before_save: :is_it_mine?
end

class LibraryEntryPolicy < ApplicationPolicy
  def is_it_mine?
    record.user == current_user
  end
end

What you guys think?

[NuckChorris]

First problem is that current_user is a controller scope, so we can't have the model to define it automatically

A resource-level hook could probably read the user off the context (I think I actually have a current_user helper method) and set it, somehow

Second, the site can import user data from another site, and those importer libs don't have a current_user set, but still need to define a valid user_id.

To be clear, that's not going through JR (that's in a Sidekiq task, you create a ListImport resource and it runs)

[DNA]

Yes, I concluded that relying on current_user wasn't a good idea when I remembered that importing was a sidekiq task :)

[NuckChorris]

Well, the model obviously wouldn't have access to current_user and the resource isn't used by the sidekiq task

[valscion]

There's some cases where this might make sense, for example if we want to allow admins to create things on behalf of users (LibraryEntry is probably not a reasonable place for this, but there are other places where it wouldn't be unreasonable)

Ok well that's true. So for this specific case, the only legit user_id ever to be submitted is the same as the current user's ID.

Authorizing on the instance does seem to be the approach advised by Pundit, and I can see other cases where it could be useful too (is the current user allowed to make a model with these specific attributes set? For example, genres or titles)

It feels like authorizing for associations through the source record in Pundit might not be as viable as we might think. It might work for create? but it won't work for update? as we cannot set association fields before authorizing update actions as ActiveRecord implicitly saves the associations immediately when a setter is called for already existing records :sob:

user = User.first
role = Role.first
# This implicitly changes the database
user.roles = [role]
# This save is redundant and doesn't do anything
user.save

I think that venuu/jsonapi-authorization#25 is actually a feature and not a bug because of this dangerous setter functionality. The real issue we'll need to solve is venuu/jsonapi-authorization#15 which will help this case, too.

What would you say if we'd get this PR out of the door by setting the user_id on a resource level hook for now and remove the user_id from LibraryEntryResource.creatable_fields and .updatable_fields? That way you wouldn't need to have a custom authorizer class implementation at all either.

This use case, which I can somehow (how, exactly..?) test against myself, will help us validate the approach to finer grained permissions. I've got the development setup of hummingbird running on my machine so I might be able to also test these different approaches directly against your app, too.