diff --git a/lib/jsonapi/authorization/authorizing_operations_processor.rb b/lib/jsonapi/authorization/authorizing_operations_processor.rb index 75bb146b..8722e428 100644 --- a/lib/jsonapi/authorization/authorizing_operations_processor.rb +++ b/lib/jsonapi/authorization/authorizing_operations_processor.rb @@ -209,8 +209,12 @@ def operation_resource_id end end + def resource_class_for_relationship(assoc_name) + @operation.resource_klass._relationship(assoc_name).resource_klass + end + def model_class_for_relationship(assoc_name) - @operation.resource_klass._relationship(assoc_name).resource_klass._model_class + resource_class_for_relationship(assoc_name)._model_class end def related_models @@ -218,10 +222,16 @@ def related_models return [] if data.nil? [:to_one, :to_many].flat_map do |rel_type| - data[rel_type].flat_map do |assoc_name, assoc_ids| - assoc_klass = model_class_for_relationship(assoc_name) - # TODO: find_by_key? - assoc_klass.find(assoc_ids) + data[rel_type].flat_map do |assoc_name, assoc_value| + case assoc_value + when Hash # polymorphic relationship + resource_class = @operation.resource_klass.resource_for(assoc_value[:type].to_s) + resource_class.find_by_key(assoc_value[:id], context: @operation.options[:context])._model + else + resource_class = resource_class_for_relationship(assoc_name) + primary_key = resource_class._primary_key + resource_class._model_class.where(primary_key => assoc_value) + end end end end diff --git a/spec/dummy/app/controllers/tags_controller.rb b/spec/dummy/app/controllers/tags_controller.rb new file mode 100644 index 00000000..4b6a4312 --- /dev/null +++ b/spec/dummy/app/controllers/tags_controller.rb @@ -0,0 +1,23 @@ +class TagsController < ActionController::Base + include JSONAPI::ActsAsResourceController + rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized + + private + + def context + {user: nil} + end + + # https://github.com/cerebris/jsonapi-resources/pull/573 + def handle_exceptions(e) + if JSONAPI.configuration.exception_class_whitelist.any? { |k| e.class.ancestors.include?(k) } + raise e + else + super + end + end + + def user_not_authorized + head :forbidden + end +end diff --git a/spec/dummy/app/models/article.rb b/spec/dummy/app/models/article.rb index 87a89fb4..aab107ab 100644 --- a/spec/dummy/app/models/article.rb +++ b/spec/dummy/app/models/article.rb @@ -1,4 +1,9 @@ class Article < ActiveRecord::Base has_many :comments + has_many :tags, as: :taggable belongs_to :author, class_name: 'User' + + def to_param + external_id + end end diff --git a/spec/dummy/app/models/comment.rb b/spec/dummy/app/models/comment.rb index e2646a32..fc350d80 100644 --- a/spec/dummy/app/models/comment.rb +++ b/spec/dummy/app/models/comment.rb @@ -1,3 +1,4 @@ class Comment < ActiveRecord::Base + has_many :tags, as: :taggable belongs_to :article end diff --git a/spec/dummy/app/models/tag.rb b/spec/dummy/app/models/tag.rb new file mode 100644 index 00000000..fb52ed7b --- /dev/null +++ b/spec/dummy/app/models/tag.rb @@ -0,0 +1,3 @@ +class Tag < ActiveRecord::Base + belongs_to :taggable, polymorphic: true +end diff --git a/spec/dummy/app/policies/tag_policy.rb b/spec/dummy/app/policies/tag_policy.rb new file mode 100644 index 00000000..b2260afb --- /dev/null +++ b/spec/dummy/app/policies/tag_policy.rb @@ -0,0 +1,34 @@ +class TagPolicy + class Scope < Struct.new(:user, :scope) + def resolve + raise NotImplementedError + end + end + + attr_reader :user, :record + + def initialize(user, record) + @user = user + @record = record + end + + def index? + raise NotImplementedError + end + + def show? + raise NotImplementedError + end + + def create? + raise NotImplementedError + end + + def update? + raise NotImplementedError + end + + def destroy? + raise NotImplementedError + end +end diff --git a/spec/dummy/app/resources/article_resource.rb b/spec/dummy/app/resources/article_resource.rb index 675e5ab7..bcf6d432 100644 --- a/spec/dummy/app/resources/article_resource.rb +++ b/spec/dummy/app/resources/article_resource.rb @@ -2,5 +2,16 @@ class ArticleResource < JSONAPI::Resource include JSONAPI::Authorization::PunditScopedResource has_many :comments, acts_as_set: true + has_many :tags has_one :author, class_name: 'User' + + primary_key :external_id + + def self.verify_key(key, _context = nil) + key && String(key) + end + + def id=(external_id) + _model.external_id = external_id + end end diff --git a/spec/dummy/app/resources/comment_resource.rb b/spec/dummy/app/resources/comment_resource.rb index 4f2bbcf5..30caaa92 100644 --- a/spec/dummy/app/resources/comment_resource.rb +++ b/spec/dummy/app/resources/comment_resource.rb @@ -1,3 +1,6 @@ class CommentResource < JSONAPI::Resource include JSONAPI::Authorization::PunditScopedResource + + has_many :tags + has_one :article end diff --git a/spec/dummy/app/resources/tag_resource.rb b/spec/dummy/app/resources/tag_resource.rb new file mode 100644 index 00000000..6473a755 --- /dev/null +++ b/spec/dummy/app/resources/tag_resource.rb @@ -0,0 +1,5 @@ +class TagResource < JSONAPI::Resource + include JSONAPI::Authorization::PunditScopedResource + + has_one :taggable, polymorphic: true +end diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb index f6aec897..41579ecd 100644 --- a/spec/dummy/config/routes.rb +++ b/spec/dummy/config/routes.rb @@ -2,4 +2,8 @@ jsonapi_resources :articles do jsonapi_relationships end + jsonapi_resources :comments do + jsonapi_relationships + end + jsonapi_resources :tags end diff --git a/spec/dummy/db/migrate/20160125083537_create_models.rb b/spec/dummy/db/migrate/20160125083537_create_models.rb index 0fc2cc8e..f659c168 100644 --- a/spec/dummy/db/migrate/20160125083537_create_models.rb +++ b/spec/dummy/db/migrate/20160125083537_create_models.rb @@ -8,7 +8,12 @@ def change end create_table :articles do |t| + t.string :external_id, null: false t.references :author end + + create_table :tags do |t| + t.references :taggable, polymorphic: true + end end end diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb index dc0ea55b..4587d46b 100644 --- a/spec/dummy/db/schema.rb +++ b/spec/dummy/db/schema.rb @@ -14,6 +14,7 @@ ActiveRecord::Schema.define(version: 20160125083537) do create_table "articles", force: :cascade do |t| + t.string "external_id", null: false t.integer "author_id" end @@ -21,6 +22,11 @@ t.integer "article_id" end + create_table "tags", force: :cascade do |t| + t.integer "taggable_id" + t.string "taggable_type" + end + create_table "users", force: :cascade do |t| end diff --git a/spec/fixtures/articles.yml b/spec/fixtures/articles.yml index 22debcb1..dc8659d8 100644 --- a/spec/fixtures/articles.yml +++ b/spec/fixtures/articles.yml @@ -1,7 +1,11 @@ --- -article_with_comments: {} -article_without_comments: {} +article_with_comments: + external_id: 1 +article_without_comments: + external_id: 2 article_with_author: + external_id: 3 author: user_1 article_without_author: + external_id: 4 author: ~ diff --git a/spec/fixtures/tags.yml b/spec/fixtures/tags.yml new file mode 100644 index 00000000..50309dfe --- /dev/null +++ b/spec/fixtures/tags.yml @@ -0,0 +1,9 @@ +--- +tag_1: + taggable: comment_1 +tag_2: + taggable: comment_1 +tag_3: + taggable: article_with_comments +tag_4: + taggable: article_with_comments diff --git a/spec/requests/related_resources_operations_spec.rb b/spec/requests/related_resources_operations_spec.rb index 6f51dbd1..9d12e8c1 100644 --- a/spec/requests/related_resources_operations_spec.rb +++ b/spec/requests/related_resources_operations_spec.rb @@ -19,7 +19,7 @@ end describe 'GET /articles/:id/comments' do - subject(:last_response) { get("/articles/#{article.id}/comments") } + subject(:last_response) { get("/articles/#{article.external_id}/comments") } let(:article) { articles(:article_with_comments) } let(:policy_scope) { Article.all } @@ -54,7 +54,7 @@ end describe 'GET /articles/:id/author' do - subject(:last_response) { get("/articles/#{article.id}/author") } + subject(:last_response) { get("/articles/#{article.external_id}/author") } let(:article) { articles(:article_with_author) } let(:policy_scope) { Article.all } diff --git a/spec/requests/relationship_operations_spec.rb b/spec/requests/relationship_operations_spec.rb index 3138ae03..1315a7bf 100644 --- a/spec/requests/relationship_operations_spec.rb +++ b/spec/requests/relationship_operations_spec.rb @@ -26,7 +26,7 @@ before do allow_any_instance_of(CommentPolicy::Scope).to receive(:resolve).and_return(comments_policy_scope) end - subject(:last_response) { get("/articles/#{article.id}/relationships/comments") } + subject(:last_response) { get("/articles/#{article.external_id}/relationships/comments") } context 'unauthorized for show_relationship' do before { disallow_operation('show_relationship', article, nil) } @@ -52,7 +52,7 @@ end describe 'GET /articles/:id/relationships/author' do - subject(:last_response) { get("/articles/#{article.id}/relationships/author") } + subject(:last_response) { get("/articles/#{article.external_id}/relationships/author") } let(:article) { articles(:article_with_author) } let(:policy_scope) { Article.all } @@ -87,7 +87,7 @@ } EOS end - subject(:last_response) { post("/articles/#{article.id}/relationships/comments", json) } + subject(:last_response) { post("/articles/#{article.external_id}/relationships/comments", json) } let(:policy_scope) { Article.all } let(:comments_scope) { Comment.all } @@ -133,7 +133,7 @@ } EOS end - subject(:last_response) { patch("/articles/#{article.id}/relationships/comments", json) } + subject(:last_response) { patch("/articles/#{article.external_id}/relationships/comments", json) } let(:policy_scope) { Article.all } let(:comments_scope) { Comment.all } @@ -180,7 +180,7 @@ end describe 'PATCH /articles/:id/relationships/author' do - subject(:last_response) { patch("/articles/#{article.id}/relationships/author", json) } + subject(:last_response) { patch("/articles/#{article.external_id}/relationships/author", json) } let(:article) { articles(:article_with_author) } let!(:old_author) { article.author } @@ -271,7 +271,7 @@ } EOS end - subject(:last_response) { delete("/articles/#{article.id}/relationships/comments", json) } + subject(:last_response) { delete("/articles/#{article.external_id}/relationships/comments", json) } let(:policy_scope) { Article.all } let(:comments_scope) { Comment.all } @@ -321,7 +321,7 @@ end describe 'DELETE /articles/:id/relationships/author' do - subject(:last_response) { delete("/articles/#{article.id}/relationships/author") } + subject(:last_response) { delete("/articles/#{article.external_id}/relationships/author") } let(:article) { articles(:article_with_author) } let(:policy_scope) { Article.all } diff --git a/spec/requests/resource_operations_spec.rb b/spec/requests/resource_operations_spec.rb index 501baa08..eca9de5c 100644 --- a/spec/requests/resource_operations_spec.rb +++ b/spec/requests/resource_operations_spec.rb @@ -34,13 +34,13 @@ it 'returns results limited by policy scope' do expect(json_data.length).to eq(1) - expect(json_data.first["id"]).to eq(article.id.to_s) + expect(json_data.first["id"]).to eq(article.external_id) end end end describe 'GET /articles/:id' do - subject(:last_response) { get("/articles/#{article.id}") } + subject(:last_response) { get("/articles/#{article.external_id}") } let(:policy_scope) { Article.all } context 'unauthorized for show' do @@ -70,7 +70,17 @@ end describe 'POST /articles' do - subject(:last_response) { post("/articles", '{ "data": { "type": "articles" } }') } + subject(:last_response) { post("/articles", json) } + let(:json) do + <<-EOS.strip_heredoc + { + "data": { + "id": "external_id", + "type": "articles" + } + } + EOS + end context 'unauthorized for create_resource' do before { disallow_operation('create_resource', Article, []) } @@ -88,14 +98,14 @@ <<-EOS.strip_heredoc { "data": { - "id": "#{article.id}", + "id": "#{article.external_id}", "type": "articles" } } EOS end - subject(:last_response) { patch("/articles/#{article.id}", json) } + subject(:last_response) { patch("/articles/#{article.external_id}", json) } let(:policy_scope) { Article.all } context 'authorized for replace_fields' do @@ -120,7 +130,7 @@ end describe 'DELETE /articles/:id' do - subject(:last_response) { delete("/articles/#{article.id}") } + subject(:last_response) { delete("/articles/#{article.external_id}") } let(:policy_scope) { Article.all } context 'unauthorized for remove_resource' do diff --git a/spec/requests/tricky_operations_spec.rb b/spec/requests/tricky_operations_spec.rb index 8fedd699..e06eeea0 100644 --- a/spec/requests/tricky_operations_spec.rb +++ b/spec/requests/tricky_operations_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' RSpec.describe 'Tricky operations', type: :request do - include PunditStubs + include AuthorizationStubs fixtures :all let(:article) { Article.all.sample } @@ -18,27 +18,52 @@ header 'Content-Type', 'application/vnd.api+json' end - describe 'GET /articles/:id?includes=comments' do - subject(:last_response) { get("/articles/#{article.id}?includes=comments") } - let(:policy_scope) { Article.all } + describe 'POST /comments (with relationships link to articles)' do + subject(:last_response) { post("/comments", json) } + let(:json) do + <<-EOS.strip_heredoc + { + "data": { + "type": "comments", + "relationships": { + "article": { + "data": { + "id": "#{article.external_id}", + "type": "articles" + } + } + } + } + } + EOS + end - context 'authorized for show? on article' do - before { allow_action('show?', article) } + context 'authorized for create_resource on Comment and [article]' do + let(:policy_scope) { Article.where(id: article.id) } + before { allow_operation('create_resource', Comment, [article]) } - xit 'displays only comments allowed by CommentPolicy::Scope' + it { is_expected.to be_successful } + end + + context 'unauthorized for create_resource on Comment and [article]' do + let(:policy_scope) { Article.where(id: article.id) } + before { disallow_operation('create_resource', Comment, [article]) } + + it { is_expected.to be_forbidden } end end - describe 'POST /comments (with relationships link to articles)', pending: true do + describe 'POST /tags (with polymorphic relationship link to article)' do + subject(:last_response) { post("/tags", json) } let(:json) do <<-EOS.strip_heredoc { "data": { - "type": "comments", + "type": "tags", "relationships": { - "article": { + "taggable": { "data": { - "id": "1", + "id": "#{article.external_id}", "type": "articles" } } @@ -48,35 +73,42 @@ EOS end - context 'unauthorized for update? on article' do - before { allow_action('update?', article) } + context 'authorized for create_resource on Tag and [article]' do + let(:policy_scope) { Article.where(id: article.id) } + before { allow_operation('create_resource', Tag, [article]) } - xcontext 'authorized for create? on comment' do - # This is a tricky one. In real life, this is often something you may - # want to permit. However, it is difficult to model with the typical - # Pundit actions without knowing the particular use case + it { is_expected.to be_successful } + end - it { is_expected.to be_forbidden } - end + context 'unauthorized for create_resource on Tag and [article]' do + let(:policy_scope) { Article.where(id: article.id) } + before { disallow_operation('create_resource', Tag, [article]) } + + it { is_expected.to be_forbidden } end end - describe 'PATCH /articles/:id (mass-modifying relationships)', pending: true do - let(:existing) do - Article.new(comments: Comment.where(id: 3)) + describe 'PATCH /articles/:id (mass-modifying relationships)' do + let!(:new_comments) do + Array.new(2) { Comment.create } + end + let(:policy_scope) { Article.where(id: article.id) } + let(:comments_policy_scope) { Comment.all } + before do + allow_any_instance_of(CommentPolicy::Scope).to receive(:resolve).and_return(comments_policy_scope) end let(:json) do <<-EOS.strip_heredoc { "data": { - "id": "<>" + "id": "#{article.external_id}", "type": "articles", "relationships": { "comments": { "data": [ - { "type": "comments", "id": "1" }, - { "type": "comments", "id": "2" } + { "type": "comments", "id": "#{new_comments.first.id}" }, + { "type": "comments", "id": "#{new_comments.second.id}" } ] } } @@ -84,22 +116,26 @@ } EOS end + subject(:last_response) { patch("/articles/#{article.external_id}", json) } - context 'authorized for update? on article' do - before { allow_action('update?', article) } - - xcontext 'unauthorized for update? on comment 3' do - it { is_expected.to be_forbidden } - end - xcontext 'unauthorized for update? on comment 2' do - it { is_expected.to be_forbidden } - end - xcontext 'unauthorized for update? on comment 1' do - it { is_expected.to be_forbidden } - end - xcontext 'authorized for update? on comments 1,2,3' do + context 'authorized for replace_fields on article and all new records' do + context 'not limited by Comments policy scope' do + before { allow_operation('replace_fields', article, new_comments) } it { is_expected.to be_successful } end + + context 'limited by Comments policy scope' do + let(:comments_policy_scope) { Comment.where("id NOT IN (?)", new_comments.map(&:id)) } + before { allow_operation('replace_fields', article, new_comments) } + + it { pending 'DISCUSS: Should this error out somehow?'; is_expected.to be_not_found } + end + end + + context 'unauthorized for replace_fields on article and all new records' do + before { disallow_operation('replace_fields', article, new_comments) } + + it { is_expected.to be_forbidden } end end end