From fe6ab4b097b51ec8218bc3c5c03489d78be460c5 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Fri, 17 Mar 2017 15:55:28 -0400 Subject: [PATCH 01/22] allow any jsonapi-resources > 0.8.0.beta1 --- Gemfile | 2 +- jsonapi-authorization.gemspec | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 96f2e6b0..55c162ad 100644 --- a/Gemfile +++ b/Gemfile @@ -21,7 +21,7 @@ case jsonapi_resources_version when 'master' gem 'jsonapi-resources', git: 'https://github.com/cerebris/jsonapi-resources.git' when 'default' - gem 'jsonapi-resources', '0.8.0.beta1' + gem 'jsonapi-resources', '0.9' else gem 'jsonapi-resources', jsonapi_resources_version end diff --git a/jsonapi-authorization.gemspec b/jsonapi-authorization.gemspec index c82a9653..7f4f4fbc 100644 --- a/jsonapi-authorization.gemspec +++ b/jsonapi-authorization.gemspec @@ -17,7 +17,7 @@ Gem::Specification.new do |spec| spec.files = `git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(test|spec|features)/}) } spec.require_paths = ["lib"] - spec.add_dependency "jsonapi-resources", "~> 0.8.0.beta1" + spec.add_dependency "jsonapi-resources", "> 0.8.0.beta1" spec.add_dependency "pundit", "~> 1.0" spec.add_development_dependency "bundler", "~> 1.11" From e07a2ae1b2703f585ba8f72a84a09966cfe94f4a Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Fri, 17 Mar 2017 15:58:12 -0400 Subject: [PATCH 02/22] conditionally set callbacks based on method name in jsonapiresources --- .../authorization/authorizing_processor.rb | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 7bedefb2..2d1918ad 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -3,6 +3,12 @@ module JSONAPI module Authorization class AuthorizingProcessor < JSONAPI::Processor + AMBIGUOUS_CALLBACKS = [ + :create_to_many_relationship, + :replace_to_many_relationship, + :remove_to_many_relationship + ] + set_callback :find, :before, :authorize_find set_callback :show, :before, :authorize_show set_callback :show_relationship, :before, :authorize_show_relationship @@ -12,10 +18,8 @@ class AuthorizingProcessor < JSONAPI::Processor set_callback :remove_resource, :before, :authorize_remove_resource set_callback :replace_fields, :before, :authorize_replace_fields set_callback :replace_to_one_relationship, :before, :authorize_replace_to_one_relationship - set_callback :create_to_many_relationship, :before, :authorize_create_to_many_relationship - set_callback :replace_to_many_relationship, :before, :authorize_replace_to_many_relationship - set_callback :remove_to_many_relationship, :before, :authorize_remove_to_many_relationship set_callback :remove_to_one_relationship, :before, :authorize_remove_to_one_relationship + set_callback :replace_polymorphic_to_one_relationship, :before, :authorize_replace_polymorphic_to_one_relationship [ :find, @@ -28,6 +32,22 @@ class AuthorizingProcessor < JSONAPI::Processor set_callback op_name, :after, :authorize_include_directive end + def self.set_relationship_callback(singular_name, preposition, callback) + callback_hook = "#{preposition}_#{singular_name}" + + if respond_to?(callback_hook) + method_to_callback_from = singular_name + else + method_to_callback_from = singular_name.to_s + "s" + end + + set_callback method_to_callback_from, preposition, callback + end + + AMBIGUOUS_CALLBACKS.each do |op_name| + set_relationship_callback(op_name, :before, "authorize_" + op_name.to_s) + end + def authorize_include_directive return if result.is_a?(::JSONAPI::ErrorsOperationResult) resources = Array.wrap( @@ -216,6 +236,10 @@ def authorize_remove_to_one_relationship authorizer.remove_to_one_relationship(source_record, relationship_type) end + def authorize_replace_polymorphic_to_one_relationship + raise NotImplementedError + end + private def authorizer From 5e273f204c1cde70108e44d9b8b5cc6a664db424 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Fri, 17 Mar 2017 16:11:58 -0400 Subject: [PATCH 03/22] rework AuthorizingProcessor#authorize_remove_to_many_relationship --- .../authorization/authorizing_processor.rb | 30 +++++++++++-------- spec/dummy/app/resources/article_resource.rb | 4 +++ .../authorizing_processor_spec.rb | 0 spec/requests/relationship_operations_spec.rb | 7 +---- 4 files changed, 23 insertions(+), 18 deletions(-) create mode 100644 spec/jsonapi/authorization/authorizing_processor_spec.rb diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 2d1918ad..73ec3db0 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -201,28 +201,34 @@ def authorize_replace_to_many_relationship ) end - def authorize_remove_to_many_relationship + def authorize_remove_to_many_relationship source_resource = @resource_klass.find_by_key( params[:resource_id], context: context ) source_record = source_resource._model - relationship_type = params[:relationship_type].to_sym - related_resource = @resource_klass - ._relationship(relationship_type) + related_resources = @resource_klass + ._relationship(params[:relationship_type].to_sym) .resource_klass - .find_by_key( - params[:associated_key], + .find_by_keys( + params[:associated_keys], context: context ) - related_record = related_resource._model unless related_resource.nil? - authorizer.remove_to_many_relationship( - source_record, - related_record, - relationship_type - ) + # find_by_keys doesn't raise if no records, so raise explicitly here + if related_resources.empty? + fail JSONAPI::Exceptions::RecordNotFound.new(params[:associated_keys]) + else + related_records = related_resources.map { |resource| resource._model } + end + + related_records.each do |related_record| + authorizer.remove_to_many_relationship( + source_record, + related_record + ) + end end def authorize_remove_to_one_relationship diff --git a/spec/dummy/app/resources/article_resource.rb b/spec/dummy/app/resources/article_resource.rb index 97a12956..e5e709f8 100644 --- a/spec/dummy/app/resources/article_resource.rb +++ b/spec/dummy/app/resources/article_resource.rb @@ -24,4 +24,8 @@ def id=(external_id) # Setting this attribute is an easy way to test that authorizations work even # when the model has validation errors attributes :blank_value + + def self.creatable_fields(context) + super + [:id] + end end diff --git a/spec/jsonapi/authorization/authorizing_processor_spec.rb b/spec/jsonapi/authorization/authorizing_processor_spec.rb new file mode 100644 index 00000000..e69de29b diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index 62042a58..aa7076e5 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -292,8 +292,7 @@ context 'not limited by policy scopes' do before do allow_operations('remove_to_many_relationship', [ - [article, comments_to_remove.first, :comments], - [article, comments_to_remove.second, :comments] + [article, kind_of(Comment)] ]) end @@ -302,10 +301,6 @@ context 'limited by policy scope on comments' do let(:comments_scope) { Comment.none } - before do - allow_operation('remove_to_many_relationship', article, kind_of(Comment), :comments) - end - it { is_expected.to be_not_found } end From 1afcadc1569002dc6922cc6b8c4bd9b8f504588b Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 09:56:01 -0400 Subject: [PATCH 04/22] Do not attempt backwards compatibility in callback naming --- .../authorization/authorizing_processor.rb | 31 ++++--------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 73ec3db0..00b5bfe4 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -3,12 +3,6 @@ module JSONAPI module Authorization class AuthorizingProcessor < JSONAPI::Processor - AMBIGUOUS_CALLBACKS = [ - :create_to_many_relationship, - :replace_to_many_relationship, - :remove_to_many_relationship - ] - set_callback :find, :before, :authorize_find set_callback :show, :before, :authorize_show set_callback :show_relationship, :before, :authorize_show_relationship @@ -19,6 +13,9 @@ class AuthorizingProcessor < JSONAPI::Processor set_callback :replace_fields, :before, :authorize_replace_fields set_callback :replace_to_one_relationship, :before, :authorize_replace_to_one_relationship set_callback :remove_to_one_relationship, :before, :authorize_remove_to_one_relationship + 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 [ @@ -32,22 +29,6 @@ class AuthorizingProcessor < JSONAPI::Processor set_callback op_name, :after, :authorize_include_directive end - def self.set_relationship_callback(singular_name, preposition, callback) - callback_hook = "#{preposition}_#{singular_name}" - - if respond_to?(callback_hook) - method_to_callback_from = singular_name - else - method_to_callback_from = singular_name.to_s + "s" - end - - set_callback method_to_callback_from, preposition, callback - end - - AMBIGUOUS_CALLBACKS.each do |op_name| - set_relationship_callback(op_name, :before, "authorize_" + op_name.to_s) - end - def authorize_include_directive return if result.is_a?(::JSONAPI::ErrorsOperationResult) resources = Array.wrap( @@ -172,7 +153,7 @@ def authorize_replace_to_one_relationship ) end - def authorize_create_to_many_relationship + def authorize_create_to_many_relationships source_record = @resource_klass.find_by_key( params[:resource_id], context: context @@ -184,7 +165,7 @@ def authorize_create_to_many_relationship authorizer.create_to_many_relationship(source_record, related_models, relationship_type) end - def authorize_replace_to_many_relationship + def authorize_replace_to_many_relationships source_resource = @resource_klass.find_by_key( params[:resource_id], context: context @@ -201,7 +182,7 @@ def authorize_replace_to_many_relationship ) end - def authorize_remove_to_many_relationship + def authorize_remove_to_many_relationships source_resource = @resource_klass.find_by_key( params[:resource_id], context: context From a2e55f3252db5ae036b6b22de05dc50271dfd844 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 09:59:35 -0400 Subject: [PATCH 05/22] depend on ~>0.9 in gemspec --- jsonapi-authorization.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jsonapi-authorization.gemspec b/jsonapi-authorization.gemspec index 7f4f4fbc..56cff17c 100644 --- a/jsonapi-authorization.gemspec +++ b/jsonapi-authorization.gemspec @@ -17,7 +17,7 @@ Gem::Specification.new do |spec| spec.files = `git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(test|spec|features)/}) } spec.require_paths = ["lib"] - spec.add_dependency "jsonapi-resources", "> 0.8.0.beta1" + spec.add_dependency "jsonapi-resources", "~> 0.9" spec.add_dependency "pundit", "~> 1.0" spec.add_development_dependency "bundler", "~> 1.11" From 9dbc640147fd8cdc6fd3116a893422ef3a67ab77 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 10:47:45 -0400 Subject: [PATCH 06/22] fix stubs in remove_to_many_relationship setup --- lib/jsonapi/authorization/authorizing_processor.rb | 3 ++- spec/requests/relationship_operations_spec.rb | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 00b5bfe4..8763da32 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -207,7 +207,8 @@ def authorize_remove_to_many_relationships related_records.each do |related_record| authorizer.remove_to_many_relationship( source_record, - related_record + related_record, + params[:relationship_type] ) end end diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index aa7076e5..21cfaf4d 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -282,7 +282,7 @@ 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") end it { is_expected.to be_forbidden } @@ -291,9 +291,7 @@ context 'authorized for remove_to_many_relationship' do context 'not limited by policy scopes' do before do - allow_operations('remove_to_many_relationship', [ - [article, kind_of(Comment)] - ]) + allow_operation('remove_to_many_relationship', article, kind_of(Comment), "comments") end it { is_expected.to be_successful } From 8097ae64ef5cf32825cf088dbb87f8ad721cb371 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 11:24:31 -0400 Subject: [PATCH 07/22] run travis against jsonapi-resources 0.9 --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 72a34b0a..2f182a41 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,8 +2,8 @@ language: ruby cache: bundler sudo: false env: - - JSONAPI_RESOURCES_VERSION=0.8.0.beta1 RAILS_VERSION=4.1.0 - - JSONAPI_RESOURCES_VERSION=0.8.0.beta1 RAILS_VERSION=4.2.0 + - JSONAPI_RESOURCES_VERSION=0.9 RAILS_VERSION=4.1.0 + - JSONAPI_RESOURCES_VERSION=0.9 RAILS_VERSION=4.2.0 - JSONAPI_RESOURCES_VERSION=master RAILS_VERSION=4.2.0 - JSONAPI_RESOURCES_VERSION=master RAILS_VERSION=4.1.0 rvm: From b055ac74bb1b767a0ad3903fac5189e064b3c34d Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 11:32:20 -0400 Subject: [PATCH 08/22] update replace_polymorphic_to_one_relationship callback syntax --- lib/jsonapi/authorization/authorizing_processor.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 8763da32..23f227ec 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -16,7 +16,11 @@ class AuthorizingProcessor < JSONAPI::Processor 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 + set_callback( + :replace_polymorphic_to_one_relationship, + :before, + :authorize_replace_polymorphic_to_one_relationship + ) [ :find, From f306f0936a76aefc45b5a435dc2ee1c8015002ac Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 14:34:28 -0400 Subject: [PATCH 09/22] remove new file --- spec/jsonapi/authorization/authorizing_processor_spec.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 spec/jsonapi/authorization/authorizing_processor_spec.rb diff --git a/spec/jsonapi/authorization/authorizing_processor_spec.rb b/spec/jsonapi/authorization/authorizing_processor_spec.rb deleted file mode 100644 index e69de29b..00000000 From 7280547d6cf5d1ca8bcec954057b6bdc1347cefc Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 14:35:30 -0400 Subject: [PATCH 10/22] send batch of related records to authorizer instead of iterating --- lib/jsonapi/authorization/authorizing_processor.rb | 12 +++++------- .../authorization/default_pundit_authorizer.rb | 4 ++-- spec/requests/relationship_operations_spec.rb | 6 +++--- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 23f227ec..cdeaf5c5 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -208,13 +208,11 @@ def authorize_remove_to_many_relationships related_records = related_resources.map { |resource| resource._model } end - related_records.each do |related_record| - authorizer.remove_to_many_relationship( - source_record, - related_record, - params[:relationship_type] - ) - end + authorizer.remove_to_many_relationship( + source_record, + related_records, + params[:relationship_type] + ) end def authorize_remove_to_one_relationship diff --git a/lib/jsonapi/authorization/default_pundit_authorizer.rb b/lib/jsonapi/authorization/default_pundit_authorizer.rb index 05863aa1..bba48900 100644 --- a/lib/jsonapi/authorization/default_pundit_authorizer.rb +++ b/lib/jsonapi/authorization/default_pundit_authorizer.rb @@ -178,9 +178,9 @@ def replace_to_many_relationship(source_record, new_related_records, relationshi # * +source_record+ - The record whose relationship is modified # * +related_record+ - The record which will be disassociated from +source_record+ # * +relationship_type+ - The relationship type - def remove_to_many_relationship(source_record, related_record, relationship_type) + def remove_to_many_relationship(source_record, related_records, relationship_type) relationship_method = "remove_from_#{relationship_type}?" - authorize_relationship_operation(source_record, relationship_method, related_record) + authorize_relationship_operation(source_record, relationship_method, related_records) end # DELETE /resources/:id/relationships/another-resource diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index 21cfaf4d..5288da4d 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -282,7 +282,7 @@ 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") end it { is_expected.to be_forbidden } @@ -291,7 +291,7 @@ context 'authorized for remove_to_many_relationship' do context 'not limited by policy scopes' 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") end it { is_expected.to be_successful } @@ -306,7 +306,7 @@ # behaviour in that case anyway, as it might be surprising. 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) end let(:policy_scope) { Article.where.not(id: article.id) } it { is_expected.to be_not_found } From a1dc9edafbde9bcdfa8e0e07f37ceafab9ec3610 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 14:42:18 -0400 Subject: [PATCH 11/22] &:(_model) instead of assigning block variable --- lib/jsonapi/authorization/authorizing_processor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index cdeaf5c5..16fa9e0d 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -205,7 +205,7 @@ def authorize_remove_to_many_relationships if related_resources.empty? fail JSONAPI::Exceptions::RecordNotFound.new(params[:associated_keys]) else - related_records = related_resources.map { |resource| resource._model } + related_records = related_resources.map(&:_model) end authorizer.remove_to_many_relationship( From 34c4832b24f037d0012f6f065c3c6852afab0f60 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Mon, 20 Mar 2017 14:44:24 -0400 Subject: [PATCH 12/22] fix up exception syntax --- lib/jsonapi/authorization/authorizing_processor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 16fa9e0d..f5f0001c 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -203,7 +203,7 @@ def authorize_remove_to_many_relationships # find_by_keys doesn't raise if no records, so raise explicitly here if related_resources.empty? - fail JSONAPI::Exceptions::RecordNotFound.new(params[:associated_keys]) + fail JSONAPI::Exceptions::RecordNotFound, params[:associated_keys] else related_records = related_resources.map(&:_model) end From 6377da9a4c244bc5708372e867b94ad6404a5eb1 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Tue, 21 Mar 2017 10:43:01 -0400 Subject: [PATCH 13/22] remove polymorphic callback --- lib/jsonapi/authorization/authorizing_processor.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index f5f0001c..93c90aef 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -16,11 +16,6 @@ class AuthorizingProcessor < JSONAPI::Processor 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 - ) [ :find, @@ -226,10 +221,6 @@ def authorize_remove_to_one_relationship authorizer.remove_to_one_relationship(source_record, relationship_type) end - def authorize_replace_polymorphic_to_one_relationship - raise NotImplementedError - end - private def authorizer From b4b02f5d60dd79c51900b1e0eca7f14b813019c3 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Wed, 22 Mar 2017 11:38:10 -0400 Subject: [PATCH 14/22] update authorized for remove_to_many_relationship spec to expect call with args --- lib/jsonapi/authorization/authorizing_processor.rb | 7 +------ spec/requests/relationship_operations_spec.rb | 6 +++++- spec/support/authorization_stubs.rb | 7 +++++++ 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 93c90aef..cf14eba0 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -196,12 +196,7 @@ def authorize_remove_to_many_relationships context: context ) - # find_by_keys doesn't raise if no records, so raise explicitly here - if related_resources.empty? - fail JSONAPI::Exceptions::RecordNotFound, params[:associated_keys] - else - related_records = related_resources.map(&:_model) - end + related_records = related_resources.map(&:_model) authorizer.remove_to_many_relationship( source_record, diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index 5288da4d..65eb6de8 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -299,7 +299,11 @@ context 'limited by policy scope on comments' do let(:comments_scope) { Comment.none } - it { is_expected.to be_not_found } + + it "calls 'remove_to_many_relationship' with no comments in scope" do + expect_operation('remove_to_many_relationship', article, [], "comments") + last_response + end end # If this happens in real life, it's mostly a bug. We want to document the diff --git a/spec/support/authorization_stubs.rb b/spec/support/authorization_stubs.rb index 858d3939..a715fc87 100644 --- a/spec/support/authorization_stubs.rb +++ b/spec/support/authorization_stubs.rb @@ -23,4 +23,11 @@ def allow_operations(operation, operation_args) allow(AUTHORIZER_CLASS).to receive(:new).with(Hash).and_return(authorizer) end + + def expect_operation(operation, *args, authorizer: instance_double(AUTHORIZER_CLASS)) + expect(authorizer).to receive(operation).with(*args) + + allow(AUTHORIZER_CLASS).to receive(:new).with(Hash).and_return(authorizer) + authorizer + end end From d2e3a57f233d05fe49e562ebaa10ea181a74a9e8 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Wed, 22 Mar 2017 11:45:14 -0400 Subject: [PATCH 15/22] do not expect 404 when authorized for remove_to_many_relationship --- spec/requests/relationship_operations_spec.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index 65eb6de8..67050e0a 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -298,12 +298,19 @@ end context 'limited by policy scope on comments' do + before do + allow_any_instance_of(ArticlePolicy).to receive( + :remove_from_comments?).and_return(true) + end + let(:comments_scope) { Comment.none } - it "calls 'remove_to_many_relationship' with no comments in scope" do + it 'calls :remove_to_many_relationship with no comments in scope' do expect_operation('remove_to_many_relationship', article, [], "comments") last_response end + + it { is_expected.not_to be_not_found } end # If this happens in real life, it's mostly a bug. We want to document the From 373169641dbaab45f3d1e0bb502cdf406de6fed7 Mon Sep 17 00:00:00 2001 From: Sam Handler Date: Wed, 22 Mar 2017 12:37:40 -0400 Subject: [PATCH 16/22] expect successful request in relationship operations remove_to_many_relationship --- spec/requests/relationship_operations_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index 67050e0a..21b3f655 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -310,7 +310,7 @@ last_response end - it { is_expected.not_to be_not_found } + it { is_expected.to be_successful } end # If this happens in real life, it's mostly a bug. We want to document the From d4cbbd7cb8000868534bf0674f36e19fbadaaf79 Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Wed, 12 Apr 2017 10:53:16 +0300 Subject: [PATCH 17/22] Use symbols for relationship_type in remove_to_many_relationships --- lib/jsonapi/authorization/authorizing_processor.rb | 6 ++++-- spec/requests/relationship_operations_spec.rb | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/jsonapi/authorization/authorizing_processor.rb b/lib/jsonapi/authorization/authorizing_processor.rb index 259589b7..6916f293 100644 --- a/lib/jsonapi/authorization/authorizing_processor.rb +++ b/lib/jsonapi/authorization/authorizing_processor.rb @@ -186,8 +186,10 @@ def authorize_remove_to_many_relationships ) source_record = source_resource._model + relationship_type = params[:relationship_type].to_sym + related_resources = @resource_klass - ._relationship(params[:relationship_type].to_sym) + ._relationship(relationship_type) .resource_klass .find_by_keys( params[:associated_keys], @@ -199,7 +201,7 @@ def authorize_remove_to_many_relationships authorizer.remove_to_many_relationship( source_record, related_records, - params[:relationship_type] + relationship_type ) end diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index 21b3f655..67049e98 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -282,7 +282,7 @@ context 'unauthorized for remove_to_many_relationship' do before do - disallow_operation('remove_to_many_relationship', article, kind_of(Array), "comments") + disallow_operation('remove_to_many_relationship', article, kind_of(Array), :comments) end it { is_expected.to be_forbidden } @@ -291,7 +291,7 @@ context 'authorized for remove_to_many_relationship' do context 'not limited by policy scopes' do before do - allow_operation('remove_to_many_relationship', article, kind_of(Array), "comments") + allow_operation('remove_to_many_relationship', article, kind_of(Array), :comments) end it { is_expected.to be_successful } @@ -306,7 +306,7 @@ let(:comments_scope) { Comment.none } it 'calls :remove_to_many_relationship with no comments in scope' do - expect_operation('remove_to_many_relationship', article, [], "comments") + expect_operation('remove_to_many_relationship', article, [], :comments) last_response end From b8e397d320e418ead70fa7ec5b120f06720c859d Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Wed, 12 Apr 2017 10:56:02 +0300 Subject: [PATCH 18/22] Expect exact models in stubbing operations in relationship specs --- spec/requests/relationship_operations_spec.rb | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index 67049e98..b9dd0b8e 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -282,7 +282,12 @@ context 'unauthorized for remove_to_many_relationship' do before do - disallow_operation('remove_to_many_relationship', article, kind_of(Array), :comments) + disallow_operation( + 'remove_to_many_relationship', + article, + [comments_to_remove.first, comments_to_remove.second], + :comments + ) end it { is_expected.to be_forbidden } @@ -291,7 +296,12 @@ context 'authorized for remove_to_many_relationship' do context 'not limited by policy scopes' do before do - allow_operation('remove_to_many_relationship', article, kind_of(Array), :comments) + allow_operation( + 'remove_to_many_relationship', + article, + [comments_to_remove.first, comments_to_remove.second], + :comments + ) end it { is_expected.to be_successful } @@ -317,7 +327,12 @@ # behaviour in that case anyway, as it might be surprising. context 'limited by policy scope on articles' do before do - allow_operation('remove_to_many_relationship', article, kind_of(Array), :comments) + allow_operation( + 'remove_to_many_relationship', + article, + [comments_to_remove.first, comments_to_remove.second], + :comments + ) end let(:policy_scope) { Article.where.not(id: article.id) } it { is_expected.to be_not_found } From 13924b75e3d15491277db07fc8342617e61220d0 Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Wed, 12 Apr 2017 11:00:11 +0300 Subject: [PATCH 19/22] Modify to-many relationship spec to only expect authorizer usage --- spec/requests/relationship_operations_spec.rb | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index b9dd0b8e..a655e0c6 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -308,16 +308,9 @@ end context 'limited by policy scope on comments' do - before do - allow_any_instance_of(ArticlePolicy).to receive( - :remove_from_comments?).and_return(true) - end - let(:comments_scope) { Comment.none } - - it 'calls :remove_to_many_relationship with no comments in scope' do - expect_operation('remove_to_many_relationship', article, [], :comments) - last_response + before do + allow_operation('remove_to_many_relationship', article, [], :comments) end it { is_expected.to be_successful } From 2a5e67b9552bf260d5095a9fea54a8ab13c909e8 Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Wed, 12 Apr 2017 11:00:30 +0300 Subject: [PATCH 20/22] Remove unnecessary expect_operation authorization stub method --- spec/support/authorization_stubs.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/spec/support/authorization_stubs.rb b/spec/support/authorization_stubs.rb index a715fc87..858d3939 100644 --- a/spec/support/authorization_stubs.rb +++ b/spec/support/authorization_stubs.rb @@ -23,11 +23,4 @@ def allow_operations(operation, operation_args) allow(AUTHORIZER_CLASS).to receive(:new).with(Hash).and_return(authorizer) end - - def expect_operation(operation, *args, authorizer: instance_double(AUTHORIZER_CLASS)) - expect(authorizer).to receive(operation).with(*args) - - allow(AUTHORIZER_CLASS).to receive(:new).with(Hash).and_return(authorizer) - authorizer - end end From a8e8df3e93cb76f210d5e2202eb397c6dc628107 Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Wed, 12 Apr 2017 11:01:58 +0300 Subject: [PATCH 21/22] Update DefaultPunditAuthorizer#remove_to_many_relationship documentation --- lib/jsonapi/authorization/default_pundit_authorizer.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/jsonapi/authorization/default_pundit_authorizer.rb b/lib/jsonapi/authorization/default_pundit_authorizer.rb index aa658053..7b99b3eb 100644 --- a/lib/jsonapi/authorization/default_pundit_authorizer.rb +++ b/lib/jsonapi/authorization/default_pundit_authorizer.rb @@ -165,12 +165,10 @@ def replace_to_many_relationship(source_record, new_related_records, relationshi # # A request to disassociate elements of a +has_many+ association # - # NOTE: this is called once per related record, not all at once - # # ==== Parameters # # * +source_record+ - The record whose relationship is modified - # * +related_record+ - The record which will be disassociated from +source_record+ + # * +related_records+ - The records which will be disassociated from +source_record+ # * +relationship_type+ - The relationship type def remove_to_many_relationship(source_record, related_records, relationship_type) relationship_method = "remove_from_#{relationship_type}?" From 927dd2021bc10c29b0c2a86a210d963ab8a88c26 Mon Sep 17 00:00:00 2001 From: Vesa Laakso Date: Wed, 12 Apr 2017 11:04:56 +0300 Subject: [PATCH 22/22] Add comment to a bit strange successful scenario in specs --- spec/requests/relationship_operations_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index a655e0c6..51e5e3f4 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -313,6 +313,8 @@ allow_operation('remove_to_many_relationship', article, [], :comments) end + # This succeeds because the request isn't actually able to try removing any comments + # due to the comments-to-be-removed being an empty array it { is_expected.to be_successful } end