venuu / jsonapi-authorization

Authorization for JSONAPI::Resource
http://www.rubydoc.info/github/venuu/jsonapi-authorization
MIT License
132 stars 59 forks source link

Show related resources doesn't check related resource policy #111

Closed Matthijsy closed 5 years ago

Matthijsy commented 5 years ago

Hi,

I just found out that when accessing the relationships of a model (for example /users/1/addresses) with a has_many relationship only the model#show? policy is checked. I would expect that also the relationship policy is checked (in my example addresses#show?). What is the reason that this doesn't happen? Is this an error or is this intended?

In my user addresses example I cannot forbid a user to get addresses of all the other users without forbidding all users to see another user. This is not what I want. How can I make this happen?

valscion commented 5 years ago

When you're accessing something plural, like in your case "addresses", jsnonapi-authorization should call addresses#index? and also make the returned addresses records go through your policy scope.

Could you tell me a bit more about what you see happening?

Matthijsy commented 5 years ago

I have a user which has many addresses, I defined all the policy methods of addresses as false (for debugging). When I do a request to /addresses I get 403. However when I do /users/1/addresses I get the list of addresses. Do I need to include some mixin? I have included JSONAPI::Authorization::PunditScopedResource

When looking in the authorizer class I see some difference between /resource/:id/relationship and /resource?include=relationship. Is this maybe cause by that? Because when i do /user?include=addresses I do get a 403.

Reference: https://github.com/venuu/jsonapi-authorization/blob/master/lib/jsonapi/authorization/default_pundit_authorizer.rb#L80 https://github.com/venuu/jsonapi-authorization/blob/master/lib/jsonapi/authorization/default_pundit_authorizer.rb#L226

valscion commented 5 years ago

Hmm yeah, PunditScopedResource should be enough to make it go through your policy scope:

https://github.com/venuu/jsonapi-authorization/blob/8176e8919c975cbe69bb548861921a197181ef38/lib/jsonapi/authorization/pundit_scoped_resource.rb#L22-L24

Have you perhaps overridden the definition for records_for in your UserResource or somewhere else?

valscion commented 5 years ago

The specs here:

https://github.com/venuu/jsonapi-authorization/blob/8176e8919c975cbe69bb548861921a197181ef38/spec/requests/related_resources_operations_spec.rb#L21-L54

Should say that jsonapi-authorization does apply the policy scope:

https://github.com/venuu/jsonapi-authorization/blob/8176e8919c975cbe69bb548861921a197181ef38/spec/requests/related_resources_operations_spec.rb#L27-L31

https://github.com/venuu/jsonapi-authorization/blob/8176e8919c975cbe69bb548861921a197181ef38/spec/requests/related_resources_operations_spec.rb#L49-L52

As otherwise the spec would've returned 3 comments instead of only 1:

https://github.com/venuu/jsonapi-authorization/blob/8176e8919c975cbe69bb548861921a197181ef38/spec/fixtures/comments.yml#L1-L13

Matthijsy commented 5 years ago

Now I see why it is not working. It is not calling AddressPolicy#index? but it's calling AddressPolicy::Scope#resolve. So I need to create a scope resolve method for the relationship. Is this the inteded behaviour?

valscion commented 5 years ago

Hmm... when viewing related resources, we do not call RelatedRecordPolicy#index? at all here:

https://github.com/venuu/jsonapi-authorization/blob/8176e8919c975cbe69bb548861921a197181ef38/lib/jsonapi/authorization/authorizing_processor.rb#L103-L110

That might be a bug. I wonder if we have the information in that place to figure out which related resource class we're trying to look at? The method should pass related_record_class: keyword argument to the authorizer if possible, and then that should be used to call:

def show_related_resources(source_record:, related_record_class:)
  ::Pundit.authorize(user, source_record, 'show?')
  ::Pundit.authorize(user, related_record_class, 'index?')
end
valscion commented 5 years ago

Now I see why it is not working. It is not calling AddressPolicy#index? but it's calling AddressPolicy::Scope#resolve. So I need to create a scope resolve method for the relationship. Is this the inteded behaviour?

Yes, you should create a scope resolve method for the relationship. I think it should've errored out if you didn't have one defined? Or maybe you have some base class that defines an "all results are OK" scope resolver?

Matthijsy commented 5 years ago

Yes we have an base Scope which returns all the records. It would be great if for related resources it will call the related record policy index? as well (as you mentioned 2 comments above). For now I will work with the Scope.resolve

valscion commented 5 years ago

Yeah sounds like a plan 😊