From 661d1e294a5a389dab3e9f073ef90cb50e5f7067 Mon Sep 17 00:00:00 2001 From: Justas Palumickas Date: Mon, 29 May 2017 12:05:56 +0300 Subject: [PATCH 1/5] Do not throw not found error when resource relationship is null --- .../authorization/authorizing_processor.rb | 8 +++++-- spec/requests/resource_operations_spec.rb | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 0856ea20..fb316fe3 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -249,6 +249,8 @@ def related_models [:to_one, :to_many].flat_map do |rel_type| data[rel_type].flat_map do |assoc_name, assoc_value| case assoc_value + when nil + next when Hash # polymorphic relationship resource_class = @resource_klass.resource_for(assoc_value[:type].to_s) resource_class.find_by_key(assoc_value[:id], context: context)._model @@ -257,7 +259,7 @@ def related_models primary_key = resource_class._primary_key resource_class._model_class.where(primary_key => assoc_value) end - end + end.compact end end @@ -269,6 +271,8 @@ def related_models_with_context data[rel_type].flat_map do |assoc_name, assoc_value| related_models = case assoc_value + when nil + next when Hash # polymorphic relationship resource_class = @resource_klass.resource_for(assoc_value[:type].to_s) resource_class.find_by_key(assoc_value[:id], context: context)._model @@ -285,7 +289,7 @@ def related_models_with_context relation_name: assoc_name, records: related_models } - end + end.compact end end diff --git a/spec/requests/resource_operations_spec.rb b/spec/requests/resource_operations_spec.rb index eca9de5c..204d9a4c 100644 --- a/spec/requests/resource_operations_spec.rb +++ b/spec/requests/resource_operations_spec.rb @@ -127,6 +127,30 @@ it { is_expected.to be_not_found } end end + + context 'nullify relationship' do + let(:article) { articles(:article_with_author) } + let(:json) do + <<-EOS.strip_heredoc + { + "data": { + "id": "#{article.external_id}", + "type": "articles", + "relationships": { "author": null } + } + } + EOS + end + let(:user_policy_scope) { User.all } + + before do + allow_any_instance_of(UserPolicy::Scope).to receive(:resolve) + .and_return(user_policy_scope) + allow_operation('replace_fields', article, []) + end + + it { is_expected.to be_successful } + end end describe 'DELETE /articles/:id' do From 5f3f37282710419bc15a23d9602fb6e90f6cee1d Mon Sep 17 00:00:00 2001 From: Justas Palumickas Date: Mon, 29 May 2017 14:43:45 +0300 Subject: [PATCH 2/5] Move tests to tricky operations --- spec/requests/resource_operations_spec.rb | 24 --------------------- spec/requests/tricky_operations_spec.rb | 26 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/spec/requests/resource_operations_spec.rb b/spec/requests/resource_operations_spec.rb index 204d9a4c..eca9de5c 100644 --- a/spec/requests/resource_operations_spec.rb +++ b/spec/requests/resource_operations_spec.rb @@ -127,30 +127,6 @@ it { is_expected.to be_not_found } end end - - context 'nullify relationship' do - let(:article) { articles(:article_with_author) } - let(:json) do - <<-EOS.strip_heredoc - { - "data": { - "id": "#{article.external_id}", - "type": "articles", - "relationships": { "author": null } - } - } - EOS - end - let(:user_policy_scope) { User.all } - - before do - allow_any_instance_of(UserPolicy::Scope).to receive(:resolve) - .and_return(user_policy_scope) - allow_operation('replace_fields', article, []) - end - - it { is_expected.to be_successful } - end end describe 'DELETE /articles/:id' do diff --git a/spec/requests/tricky_operations_spec.rb b/spec/requests/tricky_operations_spec.rb index 1eaf156a..00f6a696 100644 --- a/spec/requests/tricky_operations_spec.rb +++ b/spec/requests/tricky_operations_spec.rb @@ -168,4 +168,30 @@ it { is_expected.to be_forbidden } end end + + describe 'PATCH /articles/:id (nullifying to-one relationship)' do + let(:article) { articles(:article_with_author) } + let(:json) do + <<-EOS.strip_heredoc + { + "data": { + "id": "#{article.external_id}", + "type": "articles", + "relationships": { "author": null } + } + } + EOS + end + let(:policy_scope) { Article.all } + let(:user_policy_scope) { User.all } + subject(:last_response) { patch("/articles/#{article.external_id}", json) } + + before do + allow_any_instance_of(UserPolicy::Scope).to receive(:resolve) + .and_return(user_policy_scope) + allow_operation('replace_fields', article, []) + end + + it { is_expected.to be_successful } + end end From 8bad6887dd3c5110c46fc832735fd15e347cd356 Mon Sep 17 00:00:00 2001 From: Justas Palumickas Date: Tue, 30 May 2017 11:01:49 +0300 Subject: [PATCH 3/5] Add updates accoring to comments --- .../authorization/authorizing_processor.rb | 8 ++-- .../default_pundit_authorizer.rb | 6 ++- .../default_pundit_authorizer_spec.rb | 46 +++++++++++++++++++ spec/requests/tricky_operations_spec.rb | 9 ++-- 4 files changed, 59 insertions(+), 10 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index fb316fe3..ae1137d0 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -249,8 +249,6 @@ def related_models [:to_one, :to_many].flat_map do |rel_type| data[rel_type].flat_map do |assoc_name, assoc_value| case assoc_value - when nil - next when Hash # polymorphic relationship resource_class = @resource_klass.resource_for(assoc_value[:type].to_s) resource_class.find_by_key(assoc_value[:id], context: context)._model @@ -259,7 +257,7 @@ def related_models primary_key = resource_class._primary_key resource_class._model_class.where(primary_key => assoc_value) end - end.compact + end end end @@ -272,7 +270,7 @@ def related_models_with_context related_models = case assoc_value when nil - next + nil when Hash # polymorphic relationship resource_class = @resource_klass.resource_for(assoc_value[:type].to_s) resource_class.find_by_key(assoc_value[:id], context: context)._model @@ -289,7 +287,7 @@ def related_models_with_context relation_name: assoc_name, records: related_models } - end.compact + end end end diff --git a/lib/jsonapi/authorization/default_pundit_authorizer.rb b/lib/jsonapi/authorization/default_pundit_authorizer.rb index 0f197b7e..a1974189 100644 --- a/lib/jsonapi/authorization/default_pundit_authorizer.rb +++ b/lib/jsonapi/authorization/default_pundit_authorizer.rb @@ -260,7 +260,11 @@ def authorize_related_records(source_record, related_records_with_context) when :to_many replace_to_many_relationship(source_record, records, relation_name) when :to_one - replace_to_one_relationship(source_record, records, relation_name) + if records.nil? + remove_to_one_relationship(source_record, relation_name) + else + replace_to_one_relationship(source_record, records, relation_name) + end end end end diff --git a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb index 0b0b04a7..f0e9a63d 100644 --- a/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb +++ b/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb @@ -207,6 +207,52 @@ end end + describe 'with "relation_type: :to_one" and records is nil' do + let(:related_records_with_context) do + [{ + relation_name: :author, + relation_type: :to_one, + records: nil + }] + end + + subject(:method_call) do + -> { authorizer.replace_fields(source_record, related_records_with_context) } + end + + context 'authorized for remove_? and authorized for update? on source record' do + before { stub_policy_actions(source_record, remove_author?: true, update?: true) } + it { is_expected.not_to raise_error } + end + + context 'unauthorized for remove_? and authorized for update? on source record' do + before { stub_policy_actions(source_record, remove_author?: false, update?: true) } + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end + + context 'authorized for remove_? and unauthorized for update? on source record' do + before { stub_policy_actions(source_record, remove_author?: true, update?: false) } + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end + + context 'unauthorized for remove_? and unauthorized for update? on source record' do + before { stub_policy_actions(source_record, remove_author?: false, update?: false) } + it { is_expected.to raise_error(::Pundit::NotAuthorizedError) } + end + + context 'where remove_? 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 } + end + + 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 + end + describe 'with "relation_type: :to_many"' do let(:related_records) { Array.new(3) { Comment.new } } let(:related_records_with_context) do diff --git a/spec/requests/tricky_operations_spec.rb b/spec/requests/tricky_operations_spec.rb index 00f6a696..0cb62598 100644 --- a/spec/requests/tricky_operations_spec.rb +++ b/spec/requests/tricky_operations_spec.rb @@ -183,13 +183,14 @@ EOS end let(:policy_scope) { Article.all } - let(:user_policy_scope) { User.all } subject(:last_response) { patch("/articles/#{article.external_id}", json) } before do - allow_any_instance_of(UserPolicy::Scope).to receive(:resolve) - .and_return(user_policy_scope) - allow_operation('replace_fields', article, []) + allow_operation( + 'replace_fields', + article, + [ { relation_type: :to_one, relation_name: :author, records: nil } ] + ) end it { is_expected.to be_successful } From 5e80d28a3c98bb33f4ce631ef2e283aaf79bb1af Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Tue, 30 May 2017 11:06:53 +0300 Subject: [PATCH 4/5] Fix code style in spec --- spec/requests/tricky_operations_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/tricky_operations_spec.rb b/spec/requests/tricky_operations_spec.rb index 0cb62598..9cfb4a56 100644 --- a/spec/requests/tricky_operations_spec.rb +++ b/spec/requests/tricky_operations_spec.rb @@ -189,7 +189,7 @@ allow_operation( 'replace_fields', article, - [ { relation_type: :to_one, relation_name: :author, records: nil } ] + [{ relation_type: :to_one, relation_name: :author, records: nil }] ) end From 46edfd0aff445ade3e16a1ab8148ca3353d1c08b Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Tue, 30 May 2017 11:10:50 +0300 Subject: [PATCH 5/5] Add @jpalumickas as a contributor --- .all-contributorsrc | 11 +++++++++++ README.md | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/.all-contributorsrc b/.all-contributorsrc index be487590..156c6814 100644 --- a/.all-contributorsrc +++ b/.all-contributorsrc @@ -104,6 +104,17 @@ "code", "test" ] + }, + { + "login": "jpalumickas", + "name": "Justas Palumickas", + "avatar_url": "https://avatars0.githubusercontent.com/u/2738630?v=3", + "profile": "https://jpalumickas.com", + "contributions": [ + "bug", + "code", + "test" + ] } ] } diff --git a/README.md b/README.md index 844e44d6..597f8f5c 100644 --- a/README.md +++ b/README.md @@ -179,7 +179,7 @@ Thanks goes to these wonderful people ([emoji key](https://github.com/kentcdodds | [
Vesa Laakso](http://vesalaakso.com)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=valscion) [πŸ“–](https://github.com/Venuu/jsonapi-authorization/commits?author=valscion) πŸš‡ [⚠️](https://github.com/Venuu/jsonapi-authorization/commits?author=valscion) [πŸ›](https://github.com/Venuu/jsonapi-authorization/issues?q=author%3Avalscion) πŸ’¬ πŸ‘€ | [
Emil SΓ₯gfors](https://github.com/lime)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=lime) [πŸ“–](https://github.com/Venuu/jsonapi-authorization/commits?author=lime) πŸš‡ [⚠️](https://github.com/Venuu/jsonapi-authorization/commits?author=lime) [πŸ›](https://github.com/Venuu/jsonapi-authorization/issues?q=author%3Alime) πŸ’¬ πŸ‘€ | [
Matthias Grundmann](https://github.com/matthias-g)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=matthias-g) [πŸ“–](https://github.com/Venuu/jsonapi-authorization/commits?author=matthias-g) [⚠️](https://github.com/Venuu/jsonapi-authorization/commits?author=matthias-g) πŸ’¬ | [
Thibaud Guillaume-Gentil](http://thibaud.gg)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=thibaudgg) | [
Daniel SchweighΓΆfer](http://netsteward.net)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=acid) | [
Bruno Sofiato](https://github.com/bsofiato)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=bsofiato) | [
Adam Robertson](https://github.com/arcreative)
[πŸ“–](https://github.com/Venuu/jsonapi-authorization/commits?author=arcreative) | | :---: | :---: | :---: | :---: | :---: | :---: | :---: | -| [
Greg Fisher](https://github.com/gnfisher)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=gnfisher) [⚠️](https://github.com/Venuu/jsonapi-authorization/commits?author=gnfisher) | [
Sam](http://samlh.com)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=handlers) [⚠️](https://github.com/Venuu/jsonapi-authorization/commits?author=handlers) | +| [
Greg Fisher](https://github.com/gnfisher)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=gnfisher) [⚠️](https://github.com/Venuu/jsonapi-authorization/commits?author=gnfisher) | [
Sam](http://samlh.com)
[πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=handlers) [⚠️](https://github.com/Venuu/jsonapi-authorization/commits?author=handlers) | [
Justas Palumickas](https://jpalumickas.com)
[πŸ›](https://github.com/Venuu/jsonapi-authorization/issues?q=author%3Ajpalumickas) [πŸ’»](https://github.com/Venuu/jsonapi-authorization/commits?author=jpalumickas) [⚠️](https://github.com/Venuu/jsonapi-authorization/commits?author=jpalumickas) | This project follows the [all-contributors](https://github.com/kentcdodds/all-contributors) specification. Contributions of any kind welcome! \ No newline at end of file