search

venuu / jsonapi-authorization

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

Feature/check relationship policy #113

Closed Matthijsy closed 5 years ago

Matthijsy commented 5 years ago

This PR aims to fix the issue with the related resources policy. As discussed in #111 we would expect when calling GET /resource/:id/other-resources that resource#show? policy is checked (already the case) but als other-resources#index? is checked. This PR implements that behaviour.

It is my first work with this code base so probably I forgot to add some tests or something like that, I would really like to have some feedback and improve the code.

I am also wondering if this behaviour should be applied to show_relationship as well. Since otherwise it is still possible to do GET /resource/:id/relationships/other-resource or do I interpret that method wrong? (ref: https://github.com/venuu/jsonapi-authorization/blob/master/lib/jsonapi/authorization/default_pundit_authorizer.rb#L54)

I would like to have some feedback and your ideas about this.

Fixes #111

valscion commented 5 years ago

Thanks for taking a stab at this! The test changes seem to be going to the right direction.

It woulde be super great if we could avoid loading records from the has-many association. Is there a way we could decipher the related record class without going through the model using .public_send?

Also, seems like Travis has yet again got broken on its own... sorry about that. I haven't looked into that yet, though. Last time I fixed the CI with https://github.com/venuu/jsonapi-authorization/pull/107 and I haven't touched the config since.

We'll need to figure out how to get Travis to run the tests again (even without this feature) first before we could think of merging this PR. I don't have time this weekend to do that, and might not have time to look much into this the next week, either. Any help in this area is appreciated.

Matthijsy commented 5 years ago

Thanks for your fast feedback! I've made an PR to fix the travis builds (#115 ). I've also found a way without loading all the related resources, by searching for the related resource class directly via the relationship.

valscion commented 5 years ago

Thanks! I just merged #115 so now merging master here should hopefully fix the CI failures.

I see we have this helper method called model_class_for_relationship available in the authorizing processor. Is it worthless here? I'm not sure if safe_constantize is, err..., safe, in all cases — I wonder if jsonapi-resources would be able to provide us with some "proper" API for fetching the record class behing a resource.

valscion commented 5 years ago

Amazing, green CI! :heart_eyes:

Now, is there a way we could do this without using .safe_constantize? It feels a bit too fragile to load classes based on strings. It feels like something jsonapi-resources has to be doing internally somewhere. So I wonder if we could utilize their similar code to do that? That is, of course, unless that code is totally only for private use.

Matthijsy commented 5 years ago

I suspected that the @resource_klass attribute contained the resource class of the source model and not of the related resources. It however turned out that it contained the resource of the related resource so I could just use that attribute. Can you verify that it should contain this?

valscion commented 5 years ago

Hmmh. This seems like it's complete!

I suppose we should release this with a major version bump. This is a backwards incompatible change, as it could mean people's code can suddenly get broken when we start to authorize for more cases.

Would you be open to draft a changelog entry I could post to the releases section once I merge this and release v2.0.0? You can post the changelog entry here as a comment and I can copy-paste it to the https://github.com/venuu/jsonapi-authorization/releases page with the new version.

valscion commented 5 years ago

Oh, and do we need to update docs? Do we mention this authorization case at all in the relationship docs? I'm on my mobile now and hard to check

Matthijsy commented 5 years ago

It is not mentioned in the docs, only the create/update/delete behaviour of relationships is in the docs, but that is a different case.

Would something like this be good for the changelog?

Update of relationship endpoints This version introduces a change in the checking when accessing a relationship endpoint (for example /users/1/addresses). In the previous version only the source_record.show? was checked and the relationship was scoped. Starting with this version also the relationship.index? method is checked to verify if a user is allowed to view this relationship at all.

valscion commented 5 years ago

It is not mentioned in the docs, only the create/update/delete behaviour of relationships is in the docs, but that is a different case.

Oh wow, that seems like an oversight. I'll create an issue about this, as we will likely want to document this in https://github.com/venuu/jsonapi-authorization/blob/master/docs/relationship-authorization.md

Would something like this be good for the changelog?

Yeah, that's a good direction, I like it! I think I'll go with this slightly edited version — does this look good to you?

Breaking change: Update of relationship endpoints

This version introduces a change in the checking when accessing a relationship endpoint (for example GET /users/1/addresses).

In the previous version only the source_record.show? was checked and the relationship was scoped:

UserPolicy.new(current_user, User.find(1)).show?

addresses_returned =
  AddressPolicy::Scope.new(current_user, User.find(1).addresses).resolve

Starting with this version also the relationship.index? method is checked to verify if a user is allowed to view this relationship at all:

UserPolicy.new(current_user, User.find(1)).show?

# This is the breaking change!
AddressPolicy.new(current_user, Address).index?

addresses_returned =
  AddressPolicy::Scope.new(current_user, User.find(1).addresses).resolve
Matthijsy commented 5 years ago

Yes I think that is great for the changelog

valscion commented 5 years ago

Thanks, this looks perfect! Do you mind if I add you to the all contributors table in the README?

Matthijsy commented 5 years ago

That would be nice!

valscion commented 5 years ago

@all-contributors please add @Matthijsy for bug, test and code

allcontributors[bot] commented 5 years ago

@valscion

I've put up a pull request to add @Matthijsy! :tada:

valscion commented 5 years ago

Thank you @Matthijsy for the bug report and the fix :relaxed:. I've now released v2.0.0 of jsonapi-authorization that contains this fix: https://github.com/venuu/jsonapi-authorization/releases/tag/v2.0.0