Authorize related records on #create_resource by gnfisher · Pull Request #60 · venuu/jsonapi-authorization

gnfisher · 2017-04-08T02:18:20Z

Based on issue #56.

Thecreate_resource method will check for a create_with_<type>? action on the policy. If it doesn't find one, it will pass so long as create? passes.

I am having a hard time getting specs in /spec/requests/include_resources.rb to pass. Mainly, I am struggling with the AuthorizationStubs class.

The errors I can't solve all some variant of this:

 1) including resources alongside normal operations POST /articles/:id one-level deep has_many relationship authorized for include_has_many_resource for Comment 
     Failure/Error: authorizer.create_resource(source_class, related_models_with_context)
     
       #<InstanceDouble(JSONAPI::Authorization::DefaultPunditAuthorizer) (anonymous)> received :create_resource with unexpected arguments
         expected: (Article(id: integer, external_id: string, author_id: integer, blank_value: string), [{:relation_type=>:to_one, :relation_name=>:author, :records=>#<User id: 676971206>}, {:relation_type
..._user_id: nil>, #<Comment id: 939557610, article_id: nil, author_id: nil, reviewing_user_id: nil>]}])
              got: (Article(id: integer, external_id: string, author_id: integer, blank_value: string), [{:relation_type=>:to_one, :relation_name=>:author, :records=>#<User id: 676971206>}, {:relation_type
...nts, :records=>[#<Comment id: 939557609, article_id: nil, author_id: nil, reviewing_user_id: nil>]}])
       Diff:
       @@ -5,6 +5,5 @@
          {:relation_type=>:to_many,
           :relation_name=>:comments,
           :records=>
       -    [#<Comment id: 939557609, article_id: nil, author_id: nil, reviewing_user_id: nil>,
       -     #<Comment id: 939557610, article_id: nil, author_id: nil, reviewing_user_id: nil>]}]]
       +    [#<Comment id: 939557609, article_id: nil, author_id: nil, reviewing_user_id: nil>]}]]
       
        Please stub a default value first if message might be received with other args as well. 
     Shared Example Group: :include_directive_tests called from ./spec/requests/included_resources_spec.rb:321
     # ./lib/jsonapi/authorization/authorizing_processor.rb:115:in `authorize_create_resource'
     # ./spec/requests/included_resources_spec.rb:315:in `block (3 levels) in <top (required)>'
     # ./spec/requests/included_resources_spec.rb:45:in `block (5 levels) in <top (required)>'

Basically, in the spec when a new scoping is applied, the allow_operation method conflicts with it. This is a few levels above my usual RSpec'ing and I have banged my head against the wall long enough not to feel too bad about asking for some help and guidance on getting this suite green again!

With help I can get this wrapped up quickly, I do have time available to get this done. Excuse the messy commits for now I will rebase and clean up when I have won my war against RSpec stubs.

How to handle scenario where `create_with_<type>?` is not defined?

params

gnfisher · 2017-04-08T11:51:03Z

It occurs to me that a more ideal fallback when there is no create_with_<type>? action would be to check update? on the relationship object? That's how it was handled before.

valscion · 2017-04-08T14:13:04Z

It occurs to me that a more ideal fallback when there is no create_with_? action would be to check update? on the relationship object? That's how it was handled before.

Yeah, I actually just answered over at #56 that this fallback logic of using update on the related records would make sense and would be how it was handled before 👍

Basically, in the spec when a new scoping is applied, the allow_operation method conflicts with it. This is a few levels above my usual RSpec'ing and I have banged my head against the wall long enough not to feel too bad about asking for some help and guidance on getting this suite green again!

Yeah those are some very tricky tests and stubbing logic. I'm not quite happy about them myself either, but I'll definitely be able to give you a hand figuring these out :)

If `create_with_<type>?` is undefined, check for `update?` permissions on each related record.

gnfisher · 2017-04-10T15:54:34Z

I've added the fallback to update? on the related record.

Whenever you have time to give me a hand with the specs I can get this wrapped up @valscion. Thanks!

valscion · 2017-04-10T16:39:41Z

I'll likely have time this week to look into these. I really appreciate you working on these, thank you! ☺️

Specs deeper down the complex spec of included resources change what records CommentPolicy::Scope returns. This in turn affects our ability to expect the exact objects passed to the authorizer. We're fine with relaxing the constraint in these specs as the exact models are tested in the create_resource callback test

Now we have the correct approach of handling this case as we've got the empty array of records into the authorizer

valscion · 2017-04-12T08:53:55Z

Ok I managed to fix the included resources specs and pushed directly to your branch (hopefully that's OK).

I also merged in master here, so we're good to go with version 0.9 of JR here already 😄

I also fixed the tricky operation specs that were failing, as now they can use this new relationship operation authorization properly! Amazing!

EDIT: The build is now failing only due to code style issues, those should be easy to fix — I'll add a commit for them now :)

valscion · 2017-04-12T08:55:00Z

Excuse the messy commits for now I will rebase and clean up when I have won my war against RSpec stubs.

Don't worry about WIP commits, I'll squash & merge once we want to get this merged anyway so individual commits aren't that big a deal :)

`rubocop -a` fixes these automatically :)

valscion · 2017-04-12T09:02:51Z

I think all we need here is the same as in the missing specs at #59 (comment)

Right now we no longer have tests for the fallback scenario to update?

These specs seem to cover a little less ground than what relationship record specs do. For example, this is done immediately inside #replace_to_one_relationship specs:

context 'where replace_<type>? not defined' do
  # CommentPolicy does not define #replace_article?, so #update? should determine authorization
  let(:source_record) { comments(:comment_1) }
  let(:related_records) { Article.new }
  subject(:method_call) do
    -> { authorizer.replace_to_one_relationship(source_record, related_record, :article) }
  end

  context 'authorized for update? on record' do
    before { allow_action(source_record, 'update?') }
    it { is_expected.not_to raise_error }
  end

  context 'unauthorized for update? on record' do
    before { disallow_action(source_record, 'update?') }
    it { is_expected.to raise_error(::Pundit::NotAuthorizedError) }
  end
end

Note that this isn't inside any

context 'unauthorized for update? on source record' do

blocks, so there is no extraneous stubbing going on before the specs themselves.

Do you think these specs could be made to cover a bit more ground, taking example from the relationship tests?

gnfisher · 2017-04-12T10:23:22Z

Awesome, thank you. I will take a look today and see if I can't get all the specs done on this and on #59 today and get this wrapped up!

gnfisher · 2017-04-12T19:56:10Z

I think this covers the missing scenarios. Please let me know if more is needed.

valscion

Looking very good! Just a few nitpicks left inline. Almost ready to go! 🎉 💞

valscion · 2017-04-13T05:44:32Z

-      end
+    context 'unauthorized for create? on source class and related records is empty' do
+      let(:related_records) { [] }
+      before { stub_policy_actions(source_class, create?: true) }


This case seems to conflict from the test description — should this probably be create?: false and then expect to raise an error?

valscion · 2017-04-13T05:53:40Z

+          stub_policy_actions(source_class, create?: true)
+          related_records.each { |r| stub_policy_actions(r, update?: true) }
+        end
+        it { is_expected.not_to raise_error(::Pundit::NotAuthorizedError) }


When not expecting an error, we should not expect a specific error class, as literally any other error would cause the expectation to pass. RSpec spams the test logs with this warning for these cases:

WARNING: Using expect { }.not_to raise_error(SpecificErrorClass) risks false positives, since literally any other error would cause the expectation to pass, including those raised by Ruby (e.g. NoMethodError, NameError and ArgumentError), meaning the code you are intending to test may not even get reached. Instead consider using expect {}.not_to raise_error or expect { }.to raise_error(DifferentSpecificErrorClass).

I see we've made this same mistake in #replace_fields specs, too — I'll add a similar review note to #59 adding more of these specs :)

Yeah, I should have known better on this one as just a few weeks back I was reading about this same sort of scenario. Will update!

valscion · 2017-04-13T06:02:01Z


-      context 'unauthorized for update? on any of the related records' do
-        let(:related_records) { [Comment.new(id: 1), Comment.new(id: 2)] }
+      context 'unauthorized for update? on related records' do


This might be a bit of a nitpick, but do you think it'd be a good idea to test this as

context 'unauthorized for update? on any of the related records'

...and then just stub one of the related_records to return false for update? and stub others to return true? It would be similar to what there used to be.

Yes, I will change it. It's a better test.

- Test for fallback update? authorization failing on any related not all - Dont expect specific error when expecting not to raise error

valscion

Superb, thank you!

gnfisher added 5 commits April 7, 2017 12:10

Initial commit, authorizer and pundit policy in place.

8ad4dc8

WIP:

55e8d19

How to handle scenario where `create_with_<type>?` is not defined?

WIP: specs added for create_resource in default pundit spec

14f370a

Fix error in related_models_with_context method

b4233b8

WIP: struggling with authorizer stub and changes to create_resource

eacf5e0

params

gnfisher added the help wanted label Apr 8, 2017

Add fallback on create_resource relationship auth

4e072d2

If `create_with_<type>?` is undefined, check for `update?` permissions on each related record.

valscion added 5 commits April 12, 2017 11:10

Merge branch 'master' into uli

2fec3ac

Fix tricky operations specs

7600654

Clean up included resources spec

30252cc

Fix mass-modifying tricky operation spec

84626d2

Now we have the correct approach of handling this case as we've got the empty array of records into the authorizer

valscion removed the help wanted label Apr 12, 2017

Fix code style

66276d1

`rubocop -a` fixes these automatically :)

valscion mentioned this pull request Apr 12, 2017

Roadmap for version 1.0 🎉 #48

Closed

12 tasks

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

This was referenced Apr 12, 2017

Bug on related_models_with_context with single resource #62

Closed

Fix error in related_models_with_context method #63

Merged

Add scenarios and reorganize specs

8d6c3fd

gnfisher added the Status: Ready for review label Apr 12, 2017

valscion mentioned this pull request Apr 13, 2017

Add spec for missing #replace_fields scenario #59

Merged

valscion requested changes Apr 13, 2017

View reviewed changes

Spec tweaks

c36f9a6

- Test for fallback update? authorization failing on any related not all - Dont expect specific error when expecting not to raise error

valscion approved these changes Apr 14, 2017

View reviewed changes

valscion merged commit 7ca89d5 into venuu:master Apr 14, 2017

This was referenced May 22, 2017

Fix create_resource authorization breaking with has_one association #66

Merged

How to authorize relationships when creating a new resource? #56

Closed

Conversation

gnfisher commented Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnfisher commented Apr 8, 2017

Uh oh!

valscion commented Apr 8, 2017

Uh oh!

gnfisher commented Apr 10, 2017

Uh oh!

valscion commented Apr 10, 2017

Uh oh!

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

Uh oh!

Uh oh!

valscion commented Apr 12, 2017

Uh oh!

valscion commented Apr 12, 2017

Uh oh!

gnfisher commented Apr 12, 2017

Uh oh!

gnfisher commented Apr 12, 2017

Uh oh!

valscion left a comment

Choose a reason for hiding this comment

Uh oh!

valscion Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

valscion Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

valscion Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

gnfisher Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

valscion Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

gnfisher Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

valscion left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

gnfisher commented Apr 8, 2017 •

edited

Loading

valscion commented Apr 12, 2017 •

edited

Loading