New operation processing

[acid]

Refactoring the authorized_operation_processor to recent changes in jsonapi_resources. This is unfinished as model_includes aren't authorized correctly.

Also we should think about renaming authorized_operation_processor.rb to authorized_processor.rb.

Also, this branch should be tested against JR@master, so it will fail horribly on travis.

[ ] fix model_includes
[ ] tell travis to run specs against master of JR
[ ] update documentation

[valscion]

Cool stuff! I'll take a look at this soon.

Is there a reason why you bumped the ruby version? Seems like JR works on Ruby >= 2.0 so we shouldn't have the need to bump ruby version on our side. The ruby version is also specified in .travis.yml file, so it would be good to keep .ruby-version and Travis in sync.

[acid]

Ah, I just bumped it because our system runs on this version. That's a good reason, I will omit this commit :)

[valscion]

[ ] tell travis to run specs against master of JR

I guess we already do that?

https://github.com/venuu/jsonapi-authorization/blob/v0.6.1/.travis.yml#L9

[valscion]

Hey @lgebhardt, you said in the JR authorization issue that you could help should we need help with the operations processor refactoring, so here we are.

It seems that processor callbacks for these operation types:

• :create_resource
• :replace_fields
• :show_related_resource
• :show_related_resources

no longer can reach out to the include directives as before. Tracing back from this library to JR, it seems that the culprit is in JSONAPI::RequestParser where include_directives is no longer being passed down:

• :create_resource
• :replace_fields
• :show_related_resource
• :show_related_resources

Is it true that showing related resources with ?include parameter no longer sideload the given resources? I.e. are these invalid JSON API requests?

• POST /articles?include=author
• PATCH /articles/1?include=author
• GET /articles/1/author?include=posts
• GET /users/1/articles?include=comments

The reason why this gems model_includes authorization tests are failing against JR master version is that inside the operation callback, params[:include_directives] is not present for the operation types I mentioned above.

Our specs work on the request level and ensure that a certain request will be checked for authorization. For these four operations, the include directives are never authorized and go through even when they shouldn't.

[valscion]

And also @acid, once we get tests passing against the master branch of JR, we should look into how we can get this to be backwards compatible with older releases. We want to support JR 0.7.0 for quite some time. Seems like the JR (operations) processor API hasn't changed much, but changed enough so that we will have to have some sort of compatibility layer in place for us to be able to make this pass against 0.7.0, 0.7.1.beta1 and current master.

I don't have much expertise on how to write such compatibility layers so if you have any ideas, or someone else has, on how to do that or how other gems handle these sorts of dependency gem API changes elegantly, I'm all ears.

[NuckChorris]

The most common approach I've seen is projects just putting a notice like:

If you are using JR 0.7.0 or older, use jsonapi-authorization 0.1.0
If you are using JR 0.8.0 or newer, user jsonapi-authorization 0.2.0

In the readme. It avoids the issue entirely.

If you need a compatibility layer, I guess you'd need to define the JSONAPI::Processor class as a subclass of ActiveRecordOperationsProcessor when it's not already supplied by the JR version, and then provide class methods to proxy set_callback calls adding _operation, in addition to various other simple proxies.

Sadly, the way JR is built makes it extremely difficult to avoid private APIs, and personally I don't think it's worth the time and effort trying to avoid the inevitable problems through complicated (and error-prone) metaprogramming hackery.

[valscion]

Thanks for the input @NuckChorris! We might go for that approach, as JR does have quite a difficult API for processors.

[acid]

Hey, sorry for being so late to the party! I think it's a good idea to take the approach @NuckChorris mentioned. But apparently JR need to bump to a new version number for that. I will provide a pull request for this.

[lgebhardt]

@acid I'm trying to get a release out ASAP. I just need to get a few PRs merged.

[acid]

@lgebhardt I just did a PR for a version bump https://github.com/cerebris/jsonapi-resources/pull/765
I didn't want to cross your plans on the release or apply more pressure on you. Would it be possible to change my PR to just increase the version number to 0.7.1 in the meantime before the release?

[valscion]

While we're waiting for JR to release a new version, we could make this PR ready for new changes, too 😄

• Remove the .ruby-version bump from this PR as it isn't necessary
  • If you're using different rubies at work, are you able to use something like rbenv to work with this repository locally? Is there anything I can help you with?
• Update .travis.yml

  • Remove required tests against JR 0.7.0 and 0.7.1.beta1

  • Add commented out required test cases for JR 0.8.0

    • (we can later uncomment them just before merging to make sure everything is OK)

    env:
    -  - JSONAPI_RESOURCES_VERSION=0.7.0 RAILS_VERSION=4.1.0
    -  - JSONAPI_RESOURCES_VERSION=0.7.0 RAILS_VERSION=4.2.0
    -  - JSONAPI_RESOURCES_VERSION=0.7.1.beta1 RAILS_VERSION=4.1.0
    -  - JSONAPI_RESOURCES_VERSION=0.7.1.beta1 RAILS_VERSION=4.2.0
    +  # - JSONAPI_RESOURCES_VERSION=0.8.0 RAILS_VERSION=4.1.0
    +  # - JSONAPI_RESOURCES_VERSION=0.8.0 RAILS_VERSION=4.2.0
       - JSONAPI_RESOURCES_VERSION=master RAILS_VERSION=4.2.0
       - JSONAPI_RESOURCES_VERSION=master RAILS_VERSION=4.1.0

• Rename the authorizing_operations_processor.rb to authorizing_processor.rb
• Update README.md:
  • Use the approach mentioned by @NuckChorris to clarify required JR version
  • Add a Upgrading from 0.6.1 to 0.7.0 section above the Installation section in README.md to describe this change
    • We should note that configuration changed from config.operations_processor to config.default_processor_klass
    • Please test whether we need to tell people to require this gem in their config so that they can use the actual class JSONAPI::Authorization::AuthorizingProcessor. I'm not 100% sure that class is defined by the point where initializers are ran
    • You could add a link pointing back to this PR
    • We can look at creating a changelog outside this PR later
  • Modify the Configuration section to match the new API
  • Remove the Known bugs section as it no longer is necessary
    • If you do this removal in a single commit, you could follow-up with another commit that links to this deletion in the new Upgrading from 0.6.1 to 0.7.0 section and documents that it is no longer necessary 😉

Thanks for all of your work on this PR! Really stoked to be able to stay compatible with unreleased upstream versions like this, great work all around! 💞

[NuckChorris]

If you're gonna try and follow along with JR, it might be good to also match numbers — so instead of bumping your current 0.6 --> 0.7 for JR 0.8 support, just skip 0.7 straight to 0.8 to line up with JR's minors (which really ought to be majors, technically speaking, but that's a whole other argument!)

Otherwise you end up that JA 0.7 supports JR 0.8 and for JR 0.7 you need JA 0.6 — my head hurts just thinking about it!

[valscion]

I think it's just coincidental that we're almost at the same version number as JR. We want to be able to iterate on this gem and not have to keep our hands tied to the JR version numbering at the same time.

It's a shame that it's quite difficult to keep compatibility with JR 0.7 with this change. However, we might be able to be compatible with new JR releases even though this gem wouldn't have to updated.

I hope it's enough to just specify the dependencies properly in our gemspec and bundler will take care of checking for any possible dependency version conflicts.

[valscion]

@acid now that JR has released version 0.8.0.beta1, this PR could be finished up to target that gem version. If you don't feel like doing the README changes I specced above, I can do those later, too :)

[acid]

First of, sorry that I didn't worked on this PR for a while. I'm a big fan of SemVer combined with something like the angular commit message conventions. Maybe this would also be something for this repo? In the meantime it couldn't hurt to jump to the same version number as JR.

I'll try to find the time to work on this tonight and let you then know @valscion :)

[acid]

@valscion I've gone through all updates as proposed and it works \o/ I'm a little bit too tired for the documentation changes, sorry for that. And I'm unsure if we actually need those, too.

[valscion]

Looks good, thanks! I can add the documentation changes later myself. Also there seems to be some funky whitespace changes in the case statements but I'll follow up with a fix for them myself. Thanks for your work here! 👍