From 246efea5376ad7fa20c8ee5ee59d01b68e86f7f7 Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Sat, 3 Jun 2017 13:02:30 +0300 Subject: [PATCH 1/8] Fall back to authorizing update? on related records MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was the functionality present before v1.0.0 and should be kept intact. It is what we want to do — changes to relationships can be though as changing the related records. --- .../authorization/default_pundit_authorizer.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/jsonapi/authorization/default_pundit_authorizer.rb b/lib/jsonapi/authorization/default_pundit_authorizer.rb index a1974189..393803f8 100644 --- a/lib/jsonapi/authorization/default_pundit_authorizer.rb +++ b/lib/jsonapi/authorization/default_pundit_authorizer.rb @@ -237,10 +237,15 @@ def include_has_one_resource(_source_record, related_record) private - def authorize_relationship_operation(source_record, relationship_method, *args) + def authorize_relationship_operation( + source_record, + relationship_method, + related_record_or_records = nil + ) policy = ::Pundit.policy(user, source_record) if policy.respond_to?(relationship_method) - unless policy.public_send(relationship_method, *args) + args = [relationship_method, related_record_or_records].reject(&:nil?) + unless policy.public_send(*args) raise ::Pundit::NotAuthorizedError, query: relationship_method, record: source_record, @@ -248,6 +253,11 @@ def authorize_relationship_operation(source_record, relationship_method, *args) end else ::Pundit.authorize(user, source_record, 'update?') + if (related_record_or_records) + Array(related_record_or_records).each do |related_record| + ::Pundit.authorize(user, related_record, 'update?') + end + end end end From 02634a6cad8b956e37c5742a401b46d7ba260cfb Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Sat, 3 Jun 2017 13:52:45 +0300 Subject: [PATCH 2/8] Assert fallback for update? on has-one replace_? case --- .../authorization/default_pundit_authorizer_spec.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb index f0e9a63d..a1e4676a 100644 --- a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb +++ b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb @@ -197,7 +197,16 @@ context 'where replace_? is undefined' do context 'authorized for update? on source record' do before { stub_policy_actions(source_record, update?: true) } - it { is_expected.not_to raise_error } + + context 'authorized for update? on new related record' do + before { stub_policy_actions(related_record, update?: true) } + it { is_expected.not_to raise_error } + end + + context 'unauthorized for update? on new related record' do + before { stub_policy_actions(related_record, update?: false) } + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end end context 'unauthorized for update? on source record' do From af615366d64e153b9d359b4c8836018de12666a3 Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Sat, 3 Jun 2017 13:56:38 +0300 Subject: [PATCH 3/8] Assert fallback for update? on has-many replace_? case --- .../default_pundit_authorizer_spec.rb | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb index a1e4676a..279daab5 100644 --- a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb +++ b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb @@ -311,7 +311,22 @@ context 'where replace_? is undefined' do context 'authorized for update? on source record' do before { stub_policy_actions(source_record, update?: true) } - it { is_expected.not_to raise_error } + + context 'authorized for update? on all new related records' do + before do + related_records.each { |r| stub_policy_actions(r, update?: true) } + end + + it { is_expected.not_to raise_error } + end + + context 'unauthorized for update? on any new related records' do + before do + stub_policy_actions(related_records.first, update?: false) + end + + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end end context 'unauthorized for update? on source record' do From 2de5e8dcae89e21c27da2811c238b01e55f0f6bc Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Sat, 3 Jun 2017 14:00:23 +0300 Subject: [PATCH 4/8] Replace replace_to_one_relationship spec with similar tests as replace_fields --- .../default_pundit_authorizer_spec.rb | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb index 279daab5..25ff1ec7 100644 --- a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb +++ b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb @@ -501,21 +501,23 @@ it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end - context 'where replace_? 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 'where replace_? is undefined' do + context 'authorized for update? on source record' do + before { stub_policy_actions(source_record, update?: true) } - context 'authorized for update? on record' do - before { allow_action(source_record, 'update?') } - it { is_expected.not_to raise_error } + context 'authorized for update? on new related record' do + before { stub_policy_actions(related_record, update?: true) } + it { is_expected.not_to raise_error } + end + + context 'unauthorized for update? on new related record' do + before { stub_policy_actions(related_record, update?: false) } + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end end - context 'unauthorized for update? on record' do - before { disallow_action(source_record, 'update?') } + context 'unauthorized for update? on source record' do + before { stub_policy_actions(source_record, update?: false) } it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end end From 84d0aeddeac6f8a7df4a5b4c6be181eaef69c85d Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Sat, 3 Jun 2017 14:02:39 +0300 Subject: [PATCH 5/8] Replace replace_to_many_relationship spec with similar tests as replace_fields --- .../default_pundit_authorizer_spec.rb | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb index 25ff1ec7..94d29cec 100644 --- a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb +++ b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb @@ -596,19 +596,28 @@ end context 'where replace_? not defined' do - # ArticlePolicy does not define #replace_tags?, so #update? should determine authorization - let(:new_tags) { Array.new(3) { Tag.new } } - subject(:method_call) do - -> { authorizer.replace_to_many_relationship(article, new_tags, :tags) } - end - context 'authorized for update? on record' do - before { allow_action(article, 'update?') } - it { is_expected.not_to raise_error } + before { stub_policy_actions(article, update?: true) } + + context 'authorized for update? on all new related records' do + before do + new_comments.each { |r| stub_policy_actions(r, update?: true) } + end + + it { is_expected.not_to raise_error } + end + + context 'unauthorized for update? on any new related records' do + before do + stub_policy_actions(new_comments.first, update?: false) + end + + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end end context 'unauthorized for update? on record' do - before { disallow_action(article, 'update?') } + before { stub_policy_actions(article, update?: false) } it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end end From 8a4c803bf5542bb9e7c97a67ca0556616c87271c Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Sat, 3 Jun 2017 14:04:59 +0300 Subject: [PATCH 6/8] Replace add_to_? spec with similar tests as replace_fields --- .../default_pundit_authorizer_spec.rb | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb index 94d29cec..8a05a3c4 100644 --- a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb +++ b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb @@ -550,19 +550,28 @@ end context 'where add_to_? not defined' do - # ArticlePolicy does not define #add_to_tags?, so #update? should determine authorization - let(:related_records) { Array.new(3) { Tag.new } } - subject(:method_call) do - -> { authorizer.create_to_many_relationship(source_record, related_records, :tags) } - end - context 'authorized for update? on record' do - before { allow_action(source_record, 'update?') } - it { is_expected.not_to raise_error } + before { stub_policy_actions(source_record, update?: true) } + + context 'authorized for update? on all new related records' do + before do + related_records.each { |r| stub_policy_actions(r, update?: true) } + end + + it { is_expected.not_to raise_error } + end + + context 'unauthorized for update? on any new related records' do + before do + stub_policy_actions(related_records.first, update?: false) + end + + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end end context 'unauthorized for update? on record' do - before { disallow_action(source_record, 'update?') } + before { stub_policy_actions(source_record, update?: false) } it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end end From 9c14c816e41b02f2ece842eb20b29643d5828d5b Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Sun, 4 Jun 2017 12:25:43 +0300 Subject: [PATCH 7/8] Consolidate DefaultPunditAuthorizer update? fallback specs --- .../default_pundit_authorizer_spec.rb | 163 ++++++------------ 1 file changed, 54 insertions(+), 109 deletions(-) diff --git a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb index 8a05a3c4..1ddbaacc 100644 --- a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb +++ b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb @@ -7,6 +7,38 @@ let(:source_record) { Article.new } let(:authorizer) { described_class.new({}) } + shared_examples_for :update_singular_fallback do |related_record_method| + context 'authorized for update? on related record' do + before { stub_policy_actions(send(related_record_method), update?: true) } + + it { is_expected.not_to raise_error } + end + + context 'unauthorized for update? on related record' do + before { stub_policy_actions(send(related_record_method), update?: false) } + + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end + end + + shared_examples_for :update_multiple_fallback do |related_records_method| + context 'authorized for update? on all related records' do + before do + send(related_records_method).each { |r| stub_policy_actions(r, update?: true) } + end + + it { is_expected.not_to raise_error } + end + + context 'unauthorized for update? on any related records' do + before do + stub_policy_actions(send(related_records_method).first, update?: false) + end + + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end + end + describe '#find' do subject(:method_call) do -> { authorizer.find(source_record) } @@ -197,16 +229,7 @@ context 'where replace_? is undefined' do context 'authorized for update? on source record' do before { stub_policy_actions(source_record, update?: true) } - - context 'authorized for update? on new related record' do - before { stub_policy_actions(related_record, update?: true) } - it { is_expected.not_to raise_error } - end - - context 'unauthorized for update? on new related record' do - before { stub_policy_actions(related_record, update?: false) } - it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } - end + include_examples :update_singular_fallback, :related_record end context 'unauthorized for update? on source record' do @@ -311,22 +334,7 @@ context 'where replace_? is undefined' do context 'authorized for update? on source record' do before { stub_policy_actions(source_record, update?: true) } - - context 'authorized for update? on all new related records' do - before do - related_records.each { |r| stub_policy_actions(r, update?: true) } - end - - it { is_expected.not_to raise_error } - end - - context 'unauthorized for update? on any new related records' do - before do - stub_policy_actions(related_records.first, update?: false) - end - - it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } - end + include_examples :update_multiple_fallback, :related_records end context 'unauthorized for update? on source record' do @@ -372,20 +380,14 @@ it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end - context 'authorized for create? where create_with_? is undefined' do - context 'authorized for update? on related record' do - before do - stub_policy_actions(source_class, create?: true) - stub_policy_actions(related_record, update?: true) - end - it { is_expected.not_to raise_error } + context 'where create_with_? is undefined' do + context 'authorized for create? on source class' do + before { stub_policy_actions(source_class, create?: true) } + include_examples :update_singular_fallback, :related_record end - context 'unauthorized for update? on related record' do - before do - stub_policy_actions(source_class, create?: true) - stub_policy_actions(related_record, update?: false) - end + context 'unauthorized for create? on source class' do + before { stub_policy_actions(source_class, create?: false) } it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end end @@ -439,20 +441,14 @@ it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end - context 'authorized for create? where create_with_? is undefined' do - context 'authorized for update? on related records' do - before do - 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 } + context 'where create_with_? is undefined' do + context 'authorized for create? on source class' do + before { stub_policy_actions(source_class, create?: true) } + include_examples :update_multiple_fallback, :related_records end - context 'unauthorized for update? on any related records' do - before do - stub_policy_actions(source_class, create?: true) - stub_policy_actions(related_records.first, update?: false) - end + context 'unauthorized for create? on source class' do + before { stub_policy_actions(source_class, create?: false) } it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end end @@ -504,16 +500,7 @@ context 'where replace_? is undefined' do context 'authorized for update? on source record' do before { stub_policy_actions(source_record, update?: true) } - - context 'authorized for update? on new related record' do - before { stub_policy_actions(related_record, update?: true) } - it { is_expected.not_to raise_error } - end - - context 'unauthorized for update? on new related record' do - before { stub_policy_actions(related_record, update?: false) } - it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } - end + include_examples :update_singular_fallback, :related_record end context 'unauthorized for update? on source record' do @@ -552,22 +539,7 @@ context 'where add_to_? not defined' do context 'authorized for update? on record' do before { stub_policy_actions(source_record, update?: true) } - - context 'authorized for update? on all new related records' do - before do - related_records.each { |r| stub_policy_actions(r, update?: true) } - end - - it { is_expected.not_to raise_error } - end - - context 'unauthorized for update? on any new related records' do - before do - stub_policy_actions(related_records.first, update?: false) - end - - it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } - end + include_examples :update_multiple_fallback, :related_records end context 'unauthorized for update? on record' do @@ -607,22 +579,7 @@ context 'where replace_? not defined' do context 'authorized for update? on record' do before { stub_policy_actions(article, update?: true) } - - context 'authorized for update? on all new related records' do - before do - new_comments.each { |r| stub_policy_actions(r, update?: true) } - end - - it { is_expected.not_to raise_error } - end - - context 'unauthorized for update? on any new related records' do - before do - stub_policy_actions(new_comments.first, update?: false) - end - - it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } - end + include_examples :update_multiple_fallback, :new_comments end context 'unauthorized for update? on record' do @@ -660,19 +617,13 @@ end context 'where remove_from_? not defined' do - # ArticlePolicy does not define #remove_from_tags?, so #update? should determine authorization - let(:tags_to_remove) { article.tags.limit(2) } - subject(:method_call) do - -> { authorizer.create_to_many_relationship(article, tags_to_remove, :tags) } - end - context 'authorized for update? on article' do - before { allow_action(article, 'update?') } - it { is_expected.not_to raise_error } + before { stub_policy_actions(article, update?: true) } + include_examples :update_multiple_fallback, :comments_to_remove end context 'unauthorized for update? on article' do - before { disallow_action(article, 'update?') } + before { stub_policy_actions(article, update?: false) } it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end end @@ -704,19 +655,13 @@ end context 'where remove_? not defined' do - # CommentPolicy does not define #remove_article?, so #update? should determine authorization - let(:source_record) { comments(:comment_1) } - subject(:method_call) do - -> { authorizer.remove_to_one_relationship(source_record, :article) } - end - context 'authorized for update? on record' do - before { allow_action(source_record, 'update?') } + before { stub_policy_actions(source_record, update?: true) } it { is_expected.not_to raise_error } end context 'unauthorized for update? on record' do - before { disallow_action(source_record, 'update?') } + before { stub_policy_actions(source_record, update?: false) } it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } end end From 13e4849fd12bc151a9a79306fd0e020ecb87b578 Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Sun, 4 Jun 2017 12:51:37 +0300 Subject: [PATCH 8/8] Fix code style --- lib/jsonapi/authorization/default_pundit_authorizer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/jsonapi/authorization/default_pundit_authorizer.rb b/lib/jsonapi/authorization/default_pundit_authorizer.rb index 393803f8..a2502e3c 100644 --- a/lib/jsonapi/authorization/default_pundit_authorizer.rb +++ b/lib/jsonapi/authorization/default_pundit_authorizer.rb @@ -253,7 +253,7 @@ def authorize_relationship_operation( end else ::Pundit.authorize(user, source_record, 'update?') - if (related_record_or_records) + if related_record_or_records Array(related_record_or_records).each do |related_record| ::Pundit.authorize(user, related_record, 'update?') end