Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
fe6ab4b
allow any jsonapi-resources > 0.8.0.beta1
plantfansam Mar 17, 2017
e07a2ae
conditionally set callbacks based on method name in jsonapiresources
plantfansam Mar 17, 2017
5e273f2
rework AuthorizingProcessor#authorize_remove_to_many_relationship
plantfansam Mar 17, 2017
1afcadc
Do not attempt backwards compatibility in callback naming
plantfansam Mar 20, 2017
a2e55f3
depend on ~>0.9 in gemspec
plantfansam Mar 20, 2017
9dbc640
fix stubs in remove_to_many_relationship setup
plantfansam Mar 20, 2017
8097ae6
run travis against jsonapi-resources 0.9
plantfansam Mar 20, 2017
b055ac7
update replace_polymorphic_to_one_relationship callback syntax
plantfansam Mar 20, 2017
f306f09
remove new file
plantfansam Mar 20, 2017
7280547
send batch of related records to authorizer instead of iterating
plantfansam Mar 20, 2017
a1dc9ed
&:(_model) instead of assigning block variable
plantfansam Mar 20, 2017
34c4832
fix up exception syntax
plantfansam Mar 20, 2017
6377da9
remove polymorphic callback
plantfansam Mar 21, 2017
b4b02f5
update authorized for remove_to_many_relationship spec to expect call…
plantfansam Mar 22, 2017
d2e3a57
do not expect 404 when authorized for remove_to_many_relationship
plantfansam Mar 22, 2017
3731696
expect successful request in relationship operations remove_to_many_r…
plantfansam Mar 22, 2017
6039d71
Merge branch 'master' into jsonapi-resources-0.9
valscion Apr 12, 2017
d4cbbd7
Use symbols for relationship_type in remove_to_many_relationships
valscion Apr 12, 2017
b8e397d
Expect exact models in stubbing operations in relationship specs
valscion Apr 12, 2017
13924b7
Modify to-many relationship spec to only expect authorizer usage
valscion Apr 12, 2017
2a5e67b
Remove unnecessary expect_operation authorization stub method
valscion Apr 12, 2017
a8e8df3
Update DefaultPunditAuthorizer#remove_to_many_relationship documentation
valscion Apr 12, 2017
927dd20
Add comment to a bit strange successful scenario in specs
valscion Apr 12, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
rvm:
- 2.1.2
before_install: gem install bundler -v 1.11.2
Expand Down
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ end

case jsonapi_resources_version
when 'default'
gem 'jsonapi-resources', '0.8.0.beta1'
gem 'jsonapi-resources', '0.9'
else
gem 'jsonapi-resources', jsonapi_resources_version
end
2 changes: 1 addition & 1 deletion jsonapi-authorization.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
24 changes: 13 additions & 11 deletions lib/jsonapi/authorization/authorizing_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ 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 :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

[
:find,
Expand Down Expand Up @@ -150,7 +150,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
Expand All @@ -162,7 +162,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
Expand All @@ -179,26 +179,28 @@ 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
)
source_record = source_resource._model

relationship_type = params[:relationship_type].to_sym
related_resource = @resource_klass

related_resources = @resource_klass
._relationship(relationship_type)
.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?

related_records = related_resources.map(&:_model)

authorizer.remove_to_many_relationship(
source_record,
related_record,
related_records,
relationship_type
)
end
Expand Down
8 changes: 3 additions & 5 deletions lib/jsonapi/authorization/default_pundit_authorizer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -165,16 +165,14 @@ 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_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

# <tt>DELETE /resources/:id/relationships/another-resource</tt>
Expand Down
4 changes: 4 additions & 0 deletions spec/dummy/app/resources/article_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 👍

Copy link
Member

@valscion valscion Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆒 thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
30 changes: 22 additions & 8 deletions spec/requests/relationship_operations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,12 @@

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,
[comments_to_remove.first, comments_to_remove.second],
:comments
)
end

it { is_expected.to be_forbidden }
Expand All @@ -291,10 +296,12 @@
context 'authorized for remove_to_many_relationship' do
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]
])
allow_operation(
'remove_to_many_relationship',
article,
[comments_to_remove.first, comments_to_remove.second],
:comments
)
end

it { is_expected.to be_successful }
Expand All @@ -303,17 +310,24 @@
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)
allow_operation('remove_to_many_relationship', article, [], :comments)
end

it { is_expected.to be_not_found }
# 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

# If this happens in real life, it's mostly a bug. We want to document the
# 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,
[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 }
Expand Down