Make compatible with jsonapi-resources 0.9 #52

plantfansam · 2017-03-17T20:42:04Z

Hi! In an effort to make jsonapi-authorization's test suite pass against jsonapi-resources 0.9, this PR does a few things:

Conditionally sets callbacks for :create_to_many_relationship :replace_to_many_relationship :remove_to_many_relationship based on whether or not JSONAPI::Processor responds to the singular or plural version of the method (See Not compatible with v0.9.0 of jsonapi-resources #36).
Adds a callback and stub authorization method for replace_polymorphic_to_one_relationship, which is present in jsonapi-resources 0.9 and 0.8.0.beta1. I don't think this is strictly necessary for 0.9 compatibility but is good to have in my opinion.
Tries to fix up authorize_remove_to_many_relationship by updating the logic to find all related records and call authorizer.remove_to_many_relationship on each of them.

When this branch is run against the suite with jsonapi-resources at 0.8.0.beta1, there are a couple of failures. I am not sure what the backwards compatibility requirements are between point releases of the gem, but I definitely tried to stay somewhat backwards compatible in the conditional callback setting strategy.

I am super new to this project and definitely want to get some experienced 👀 on this! Happy to chop it up into separate PRs and fix stuff up, as this one has a few distinct threads to it.

valscion · 2017-03-18T10:46:10Z

Hi, and welcome! Thank you for starting a PR for this. I'm very excited to soon jump to review it!

Hopefully it's ok that this is going to take me a few days at least until I can look into this more. I've got a tight schedule at work for a while, and as my wrist is aching again I won't be able to look at this on my freetime anytime soon.

I won't forget about this PR, though :). I'll check this out when I've got a good timeslot for this 💞

valscion · 2017-03-19T18:13:36Z

Closed and reopened to try to trigger a CI build... wonder what's going on 😕

valscion · 2017-03-19T18:36:59Z

This seems to be going to a good direction, thank you!

I'm not sure whether we've got a good answer to the backwards compatibility here. The relationship processor hooks have changed a bit, and when we started to authorize for relationship actions we hit these changes, too: #40 (comment)

I'd be OK with going by just supporting 0.9 in a future release, as it would make the relationship authorizations cleaner as well. 0.9.0 is out of beta so it's fine if we only target that version, too.

Would you be OK with setting this PR up to only support 0.9 and removing the then-unnecessary checks from your changes?

plantfansam · 2017-03-19T20:22:05Z

Yes, totally ok with the route of 0.9-only compatibility! Should make the code a lot cleaner, too! 🙂

…

On Sun, Mar 19, 2017 at 2:37 PM Vesa Laakso ***@***.***> wrote: This seems to be going to a good direction, thank you! I'm not sure whether we've got a good answer to the backwards compatibility here. The relationship processor hooks have changed a bit, and when we started to authorize for relationship actions we hit these changes, too: #40 (comment) <#40 (comment)> I'd be OK with going by just supporting 0.9 in a future release, as it would make the relationship authorizations cleaner as well. 0.9.0 is out of beta so it's fine if we only target that version, too. Would you be OK with setting this PR up to only support 0.9 and removing the then-unnecessary checks from your changes? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#52 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWmBstuTcaVNt21pQFuyKfKkRtAOPZMks5rnXXMgaJpZM4MhE-Z> .

plantfansam · 2017-03-20T14:51:36Z

OK, I have updated the gemspec to require jsonapi-resources 0.9 and updated the code accordingly. I also had to update the remove_to_many_relationship calls to match the new method signature.

valscion · 2017-03-20T15:05:53Z

spec/requests/relationship_operations_spec.rb

    context 'unauthorized for remove_to_many_relationship' do
      before do
-        disallow_operation('remove_to_many_relationship', article, kind_of(Comment), :comments)
+        disallow_operation('remove_to_many_relationship', article, kind_of(Comment), "comments")


Hmm I wonder whether this change was necessary?

valscion · 2017-03-20T15:07:27Z

lib/jsonapi/authorization/authorizing_processor.rb

      end

+      def authorize_replace_polymorphic_to_one_relationship
+        raise NotImplementedError


Good call with just raising NotImplementedError here.

We'll need to figure out how to authorize for this polymorphic case before shipping v1.0.0 but that might be better to discuss outside of this PR.

Actually, I think we'd like to skip raising this here and figure out what to do with the polymorphic relationship scenario outside of this PR. This way we wouldn't add too much noise to this upgrade path.

Would you like to remove this callback from the processor?

Sure! Removed it here 6377da9

valscion · 2017-03-20T15:09:05Z

spec/dummy/app/resources/article_resource.rb

+
+  def self.creatable_fields(context)
+    super + [:id]
+  end


Hmm what was the reason for this change? Is there a reason why it is needed?

I didn't discover the reason in jsonapi-resources, but there are 22 failures if this is left out. In included_resources_spec, the failures look approximately like this: "Param not allowed", "detail" : "id is not allowed." I can search for more details if you like!

Hmm I wonder if it has got to with @thibaudgg's PR #11 and the changes done to the dummy app ArticleResource in https://github.com/venuu/jsonapi-authorization/pull/11/files#diff-cc66704bb355f2769ceb1ed76d5f02b4

I'm not really sure how the self.verify_key and id= methods work, and why exactly they were necessary to have. @thibaudgg do you have any insight to this?

Hi, I'll have a look next week and give a try to this branch with our app.
Thanks @handlers for all the work on that one! 👍

Thanks @thibaudgg! Let us know if you hit roadblocks.

These might be relevant to your testing, if you haven't updated in a while: #53 #40

I'm waiting on @thibaudgg to be able to test this and confirm it works nicely :)

🆒 thanks!

I launched our test suite on our CI with jsonapi-resource 0.9 and this PR and we get a bunch of errors (unexpected 406 - Not Acceptable responses). It's hard to tell right now if it's because of upgrading to JR 0.9 or this PR specifically.
Sadly I didn't have much time to investigate more this week. I'll try to block an hour next week.

Huh, that 406 error does indeed seem weird. It might very well be because of JR 0.9, too

Ok turns out that these lines are indeed needed since v0.8.3 of JR as IDs are generated on the client side.

See https://github.com/cerebris/jsonapi-resources/releases/tag/v0.8.3

valscion · 2017-03-20T15:13:04Z

lib/jsonapi/authorization/authorizing_processor.rb

+            source_record,
+            related_record,
+            params[:relationship_type]
+          )


Instead of looping through all the related_records, we might want to instead change the API of remove_to_many_relationship to take an array of related_records (instead of one at a time).

That way we wouldn't even have to do the if related_resource.empty? check you're doing before calling the authorizer right now, too.

There was a comment #40 (comment) that maybe related to this not being possible before 0.9 version of JR.

This makes sense to me. I will try!

valscion · 2017-03-20T15:18:31Z

You'll want to change the .travis.yml to configure Travis to use 0.9.0 version of JR when running tests

valscion · 2017-03-20T15:24:48Z

lib/jsonapi/authorization/authorizing_processor.rb

+      set_callback :create_to_many_relationships, :before, :authorize_create_to_many_relationships
+      set_callback :replace_to_many_relationships, :before, :authorize_replace_to_many_relationships
+      set_callback :remove_to_many_relationships, :before, :authorize_remove_to_many_relationships
+      set_callback :replace_polymorphic_to_one_relationship, :before, :authorize_replace_polymorphic_to_one_relationship


This line is too long and will cause the linter to fail the build. Sorry about that 😥

Maybe write it like like this?

set_callback( :replace_polymorphic_to_one_relationship, :before, :authorize_replace_polymorphic_to_one_relationship )

plantfansam · 2017-03-20T18:55:30Z

I've updated the PR to send records to the authorizer as a batch (7280547) instead of iterating over them individually. Travis is also now running successfully :). There is still an open thread related to an update required to make the dummy app's ArticleResource work with existing threads that we should resolve.

valscion · 2017-03-21T06:52:32Z

lib/jsonapi/authorization/authorizing_processor.rb

+
+        # find_by_keys doesn't raise if no records, so raise explicitly here
+        if related_resources.empty?
+          fail JSONAPI::Exceptions::RecordNotFound, params[:associated_keys]


Do we need this check here? What specs fail if we'd remove this part of the code?

Maybe those tests that would fail have incorrect behaviour which we'd like to change. I don't know, but this seems a bit out of place here.

Good question! If this is removed, one spec failure occurs: when "authorized for remove_to_many_relationship" and "limited by policy scope on comments" it { is_expected.to be_not_found }. The spec expects a 404 if the comments scope (CommentPolicy::Scope) is Comment.none.

With the new code, it instead tries to run remove_from_comments?, which raises NotImplementedError.

1) Relationship operations DELETE /articles/:id/relationships/comments authorized for remove_to_many_relationship limited by policy scope on comments Failure/Error: raise NotImplementedError NotImplementedError: NotImplementedError # ./spec/dummy/app/policies/article_policy.rb:44:in `remove_from_comments?' # ./lib/jsonapi/authorization/default_pundit_authorizer.rb:238:in `public_send' # ./lib/jsonapi/authorization/default_pundit_authorizer.rb:238:in `authorize_relationship_operation' # ./lib/jsonapi/authorization/default_pundit_authorizer.rb:183:in `remove_to_many_relationship' # ./lib/jsonapi/authorization/authorizing_processor.rb:201:in `authorize_remove_to_many_relationships' # ./spec/requests/relationship_operations_spec.rb:275:in `block (3 levels) in <top (required)>' # ./spec/requests/relationship_operations_spec.rb:302:in `block (5 levels) in <top (required)>'

I included the fail to preserve the old 404 behavior. Perhaps we could move that fail to authorize_relationship_operation?

Hmm I guess we'd really want to test that the DELETE operation authorizes properly, and doesn't use 404 when the comments can't be found.

Can you modify the tests to expect that remove_to_many_relationship authorizer method will be called with the proper arguments?

Sure! I have updated the tests! b4b02f5 d2e3a57

Thanks so much for the 👀!

… with args

valscion · 2017-03-22T16:24:48Z

spec/requests/relationship_operations_spec.rb

+          last_response
+        end
+
+        it { is_expected.not_to be_not_found }


Can we write this spec as either

it { is_expected.to be_successful }

or

it { is_expected.to be_forbidden }

like we have done with all the other specs?

If this feels a bit over your head, I could also tackle these at some point in the future.

No problem! I updated here 3731696. I am glad to keep revising the PR, but if it's easier for you to finish it, that's fine too!

…elationship

valscion

Hi! I left final review comments for the specs. If these can be addressed, we could merge this PR and fix issues that crop up with JR 0.9 in a future PR ☺️

valscion · 2017-03-31T07:14:28Z

spec/requests/relationship_operations_spec.rb

        before do
-          allow_operation('remove_to_many_relationship', article, kind_of(Comment), :comments)
+          allow_any_instance_of(ArticlePolicy).to receive(
+            :remove_from_comments?).and_return(true)


It would be great if we didn't have to worry about policies in here at all, and could create this spec in terms of only the operations stubbing instead.

Would it be possible to restructure this spec to look something like this?

context 'limited by policy scope on comments' do let(:comments_scope) { Comment.none } before do allow_operation('remove_to_many_relationship', article, [], :comments) end it { is_expected.to be_succesful } end

This would also ensure that the remove_to_many_relationship is indeed called with an empty array instead of any comments.

valscion · 2017-03-31T07:17:22Z

spec/requests/relationship_operations_spec.rb

-            [article, comments_to_remove.first, :comments],
-            [article, comments_to_remove.second, :comments]
-          ])
+          allow_operation('remove_to_many_relationship', article, kind_of(Array), "comments")


Could we restructure this to test for the actual comments instead of just kind_of(Array)? The kind_of(Array) might be a bit too vague spec and could cause bugs to slip through as it doesn't care about the type of the objects inside the array itself.

Would something like this work?

allow_operation( 'remove_to_many_relationship', article, [comments_to_remove.first, comments_to_remove.second], :comments )

That would be the best. If the specs fail because of this due to some matchers not working correctly, we should fix the matchers instead of trying to work around them by using less specific assertions.

Also, as you can see, let's try to use the symbol version for the relationship name in the assertions (:comments instead of "comments") or otherwise it might result in an inconsistent API with the other authorizers)

valscion · 2017-03-31T07:17:56Z

spec/requests/relationship_operations_spec.rb

    context 'unauthorized for remove_to_many_relationship' do
      before do
-        disallow_operation('remove_to_many_relationship', article, kind_of(Comment), :comments)
+        disallow_operation('remove_to_many_relationship', article, kind_of(Array), "comments")


Same thing as in https://github.com/venuu/jsonapi-authorization/pull/52/files#r109101102, just that the assertion should be disallow_operation instead of allow_operation

valscion · 2017-03-31T07:18:25Z

spec/requests/relationship_operations_spec.rb

      context 'limited by policy scope on articles' do
        before do
-          allow_operation('remove_to_many_relationship', article, kind_of(Comment), :comments)
+          allow_operation('remove_to_many_relationship', article, kind_of(Array), :comments)


Same as https://github.com/venuu/jsonapi-authorization/pull/52/files#r109100787

valscion · 2017-03-31T07:19:14Z

spec/support/authorization_stubs.rb

+
+    allow(AUTHORIZER_CLASS).to receive(:new).with(Hash).and_return(authorizer)
+    authorizer
+  end


We hopefully don't need to add this new method here, if we can satisfy the spec in https://github.com/venuu/jsonapi-authorization/pull/52/files#r109100787 with the existing matchers

plantfansam · 2017-04-03T13:44:29Z

Thanks so much @valscion! I am going to try to update the PR today or tomorrow.

thibaudgg · 2017-04-11T14:13:12Z

@valscion I was able to have all our specs passed with that PR, so it looks good so far for us. 👍

@handlers could you please rebase it on master? I would like to test it more now that #51 has been merged. Thanks!

valscion · 2017-04-11T14:32:05Z

Thank you so much @thibaudgg for testing this out, really appreciate it!

We actually just moments ago stumbled upon an issue in JR (it was this one if you're interested) and as it has been fixed in 0.9.0 we'll want to push this PR through if not today, then tomorrow.

could you please rebase it on master?

We'd actually prefer merge commits over rebasing. We like to keep the history of the conversation in place.

valscion · 2017-04-11T15:04:41Z

I'll tackle this tomorrow (which in our timezone would be around 16 hours from now).

So if you feel like addressing the tests before that, @handlers, feel free to, but if you don't have the time I'll happily plow through them as well and get this PR merged ☺️

Thank you so much for your efforts here! 💞

plantfansam · 2017-04-11T19:46:33Z

I won't be able to get to this for a while, so feel free to make any modifications necessary!

…

On Tue, Apr 11, 2017 at 11:04 AM Vesa Laakso ***@***.***> wrote: I'll tackle this tomorrow (which in our timezone would be around 16 hours from now). So if you feel like addressing the tests before that, @handlers <https://github.com/handlers>, feel free to, but if you don't have the time I'll happily plow through them as well and get this PR merged ☺️ Thank you so much for your efforts here! 💞 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWmBqmoCWu9QLqjL3nh1JbHxP5uupBCks5ru5aLgaJpZM4MhE-Z> .

valscion

🎉 Thank you!

thibaudgg · 2017-04-12T08:18:48Z

👏

valscion closed this Mar 19, 2017

valscion reopened this Mar 19, 2017

plantfansam added 6 commits March 20, 2017 10:02

allow any jsonapi-resources > 0.8.0.beta1

fe6ab4b

conditionally set callbacks based on method name in jsonapiresources

e07a2ae

rework AuthorizingProcessor#authorize_remove_to_many_relationship

5e273f2

Do not attempt backwards compatibility in callback naming

1afcadc

depend on ~>0.9 in gemspec

a2e55f3

fix stubs in remove_to_many_relationship setup

9dbc640

valscion reviewed Mar 20, 2017

View reviewed changes

valscion mentioned this pull request Mar 20, 2017

Roadmap for version 1.0 🎉 #48

Closed

12 tasks

run travis against jsonapi-resources 0.9

8097ae6

valscion reviewed Mar 20, 2017

View reviewed changes

plantfansam added 5 commits March 20, 2017 11:32

update replace_polymorphic_to_one_relationship callback syntax

b055ac7

remove new file

f306f09

send batch of related records to authorizer instead of iterating

7280547

&:(_model) instead of assigning block variable

a1dc9ed

fix up exception syntax

34c4832

valscion reviewed Mar 21, 2017

View reviewed changes

remove polymorphic callback

6377da9

plantfansam mentioned this pull request Mar 21, 2017

Register replace_polymorphic_to_one_relationship callback #53

Closed

plantfansam added 2 commits March 22, 2017 11:38

update authorized for remove_to_many_relationship spec to expect call…

b4b02f5

… with args

do not expect 404 when authorized for remove_to_many_relationship

d2e3a57

valscion reviewed Mar 22, 2017

View reviewed changes

expect successful request in relationship operations remove_to_many_r…

3731696

…elationship

valscion requested changes Mar 31, 2017

View reviewed changes

valscion added the Review: Feedback given label Mar 31, 2017

valscion self-assigned this Mar 31, 2017

valscion mentioned this pull request Mar 31, 2017

Handling relationship policy checking in replace_fields #51

Merged

valscion added 7 commits April 12, 2017 10:46

Merge branch 'master' into jsonapi-resources-0.9

6039d71

Use symbols for relationship_type in remove_to_many_relationships

d4cbbd7

Expect exact models in stubbing operations in relationship specs

b8e397d

Modify to-many relationship spec to only expect authorizer usage

13924b7

Remove unnecessary expect_operation authorization stub method

2a5e67b

Update DefaultPunditAuthorizer#remove_to_many_relationship documentation

a8e8df3

Add comment to a bit strange successful scenario in specs

927dd20

valscion removed the Review: Feedback given label Apr 12, 2017

valscion approved these changes Apr 12, 2017

View reviewed changes

valscion merged commit 100d6eb into venuu:master Apr 12, 2017

valscion mentioned this pull request Apr 12, 2017

Add @handlers to the list of contributors #61

Merged

d4rky-pl mentioned this pull request May 4, 2017

Not compatible with v0.9.0 of jsonapi-resources #36

Closed

Make compatible with jsonapi-resources 0.9 #52

Make compatible with jsonapi-resources 0.9 #52

Uh oh!

Conversation

plantfansam commented Mar 17, 2017

Uh oh!

valscion commented Mar 18, 2017

Uh oh!

valscion commented Mar 19, 2017

Uh oh!

valscion commented Mar 19, 2017

Uh oh!

plantfansam commented Mar 19, 2017 via email

Uh oh!

plantfansam commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valscion Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valscion commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plantfansam commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valscion left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

plantfansam commented Mar 20, 2017 •

edited

Loading

valscion Mar 24, 2017 •

edited

Loading

valscion left a comment •

edited

Loading

valscion commented Apr 11, 2017 •

edited

Loading