Check for authorization against related records (#119)

* AuthorizingProcessor#related_models_with_context should not use the scope

* Remove unused AuthorizingProcessor#related_models

* Check all related records in authorize_remove_to_many_relationships

* Return 404 instead of 403 for records out of scope

* Style fixes

* Optimization for Array#size

* Comment to clarify changes to mocks

* Better array matching in specs

Author: brianswko

lib/jsonapi/authorization/authorizing_processor.rb

@@ -219,6 +219,10 @@ def authorize_remove_to_many_relationships
 
         related_records = related_resources.map(&:_model)
 
+        if related_records.size != params[:associated_keys].uniq.size
+          fail JSONAPI::Exceptions::RecordNotFound, params[:associated_keys]
+        end
+
         authorizer.remove_to_many_relationship(
           source_record: source_record,
           related_records: related_records,
@@ -298,25 +302,6 @@ def model_class_for_relationship(assoc_name)
         resource_class_for_relationship(assoc_name)._model_class
       end
 
-      def related_models
-        data = params[:data]
-        return [] if data.nil?
-
-        [:to_one, :to_many].flat_map do |rel_type|
-          data[rel_type].flat_map do |assoc_name, assoc_value|
-            case assoc_value
-            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
-            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
-
       def related_models_with_context
         data = params[:data]
         return { relationship: nil, relation_name: nil, records: nil } if data.nil?
@@ -330,12 +315,15 @@ def related_models_with_context
               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
-              when Array
-                resource_class = resource_class_for_relationship(assoc_name)
-                resource_class.find_by_keys(assoc_value, context: context).map(&:_model)
               else
                 resource_class = resource_class_for_relationship(assoc_name)
-                resource_class.find_by_key(assoc_value, context: context)._model
+                resources = resource_class.find_by_keys(assoc_value, context: context)
+                resources.map(&:_model).tap do |scoped_records|
+                  related_ids = Array.wrap(assoc_value).uniq
+                  if scoped_records.count != related_ids.count
+                    fail JSONAPI::Exceptions::RecordNotFound, related_ids
+                  end
+                end
               end
 
             {

spec/requests/included_resources_spec.rb

@@ -7,20 +7,23 @@
   subject { last_response }
   let(:json_included) { JSON.parse(last_response.body)['included'] }
 
-  let(:comments_policy_scope) { Comment.none }
+  let(:comments_policy_scope) { Comment.all }
   let(:article_policy_scope) { Article.all }
   let(:user_policy_scope) { User.all }
 
+  # Take the stubbed scope and call merge(policy_scope.scope.all) so that the original
+  # scope's conditions are not lost. Without it, the stub will always return all records
+  # the user has access to regardless of context.
   before do
-    allow_any_instance_of(ArticlePolicy::Scope).to receive(:resolve).and_return(
-      article_policy_scope
-    )
-    allow_any_instance_of(CommentPolicy::Scope).to receive(:resolve).and_return(
-      comments_policy_scope
-    )
-    allow_any_instance_of(UserPolicy::Scope).to receive(:resolve).and_return(
-      user_policy_scope
-    )
+    allow_any_instance_of(ArticlePolicy::Scope).to receive(:resolve) do |policy_scope|
+      article_policy_scope.merge(policy_scope.scope.all)
+    end
+    allow_any_instance_of(CommentPolicy::Scope).to receive(:resolve) do |policy_scope|
+      comments_policy_scope.merge(policy_scope.scope.all)
+    end
+    allow_any_instance_of(UserPolicy::Scope).to receive(:resolve) do |policy_scope|
+      user_policy_scope.merge(policy_scope.scope.all)
+    end
   end
 
   before do
@@ -31,8 +34,6 @@
     describe 'one-level deep has_many relationship' do
       let(:include_query) { 'comments' }
 
-      let(:comments_policy_scope) { Comment.all }
-
       context 'unauthorized for include_has_many_resource for Comment' do
         before {
           disallow_operation(
@@ -58,11 +59,11 @@
 
         it { is_expected.to be_successful }
 
-        let(:comments_policy_scope) { Comment.limit(1) }
-
-        it 'includes only comments allowed by policy scope' do
-          expect(json_included.length).to eq(1)
-          expect(json_included.first["id"]).to eq(comments_policy_scope.first.id.to_s)
+        it 'includes only comments allowed by policy scope and associated with the article' do
+          expect(json_included.length).to eq(article.comments.count)
+          expect(
+            json_included.map { |included| included["id"].to_i }
+          ).to match_array(article.comments.map(&:id))
         end
       end
     end
@@ -104,7 +105,6 @@
 
     describe 'multiple one-level deep relationships' do
       let(:include_query) { 'author,comments' }
-      let(:comments_policy_scope) { Comment.all }
 
       context 'unauthorized for include_has_one_resource for article.author' do
         before do
@@ -136,12 +136,12 @@
 
         it { is_expected.to be_successful }
 
-        let(:comments_policy_scope) { Comment.limit(1) }
-
-        it 'includes only comments allowed by policy scope' do
+        it 'includes only comments allowed by policy scope and associated with the article' do
           json_comments = json_included.select { |item| item['type'] == 'comments' }
-          expect(json_comments.length).to eq(comments_policy_scope.length)
-          expect(json_comments.map { |i| i['id'] }).to eq(comments_policy_scope.pluck(:id).map(&:to_s))
+          expect(json_comments.length).to eq(article.comments.count)
+          expect(
+            json_comments.map { |i| i['id'] }
+          ).to match_array(article.comments.pluck(:id).map(&:to_s))
         end
 
         it 'includes the associated author resource' do
@@ -181,20 +181,18 @@
 
           it { is_expected.to be_successful }
 
-          let(:comments_policy_scope) { Comment.all }
-
           it 'includes the first level resource' do
             json_users = json_included.select { |item| item['type'] == 'users' }
             expect(json_users).to include(a_hash_including('id' => article.author.id.to_s))
           end
 
           describe 'second level resources' do
-            let(:comments_policy_scope) { Comment.limit(1) }
-
             it 'includes only resources allowed by policy scope' do
               second_level_items = json_included.select { |item| item['type'] == 'comments' }
-              expect(second_level_items.length).to eq(comments_policy_scope.length)
-              expect(second_level_items.map { |i| i['id'] }).to eq(comments_policy_scope.pluck(:id).map(&:to_s))
+              expect(second_level_items.length).to eq(article.author.comments.count)
+              expect(
+                second_level_items.map { |i| i['id'] }
+              ).to match_array(article.author.comments.pluck(:id).map(&:to_s))
             end
           end
         end
@@ -226,6 +224,137 @@
     end
   end
 
+  shared_examples_for :scope_limited_directive_tests do
+    describe 'one-level deep has_many relationship' do
+      let(:comments_policy_scope) { Comment.where(id: article.comments.first.id) }
+      let(:include_query) { 'comments' }
+
+      context 'authorized for include_has_many_resource for Comment' do
+        before {
+          allow_operation(
+            'include_has_many_resource',
+            source_record: an_instance_of(Article),
+            record_class: Comment,
+            authorizer: chained_authorizer
+          )
+        }
+
+        it { is_expected.to be_successful }
+
+        it 'includes only comments allowed by policy scope' do
+          expect(json_included.length).to eq(comments_policy_scope.length)
+          expect(json_included.first["id"]).to eq(comments_policy_scope.first.id.to_s)
+        end
+      end
+    end
+
+    describe 'multiple one-level deep relationships' do
+      let(:include_query) { 'author,comments' }
+      let(:comments_policy_scope) { Comment.where(id: article.comments.first.id) }
+
+      context 'authorized for both operations' do
+        before do
+          allow_operation('include_has_one_resource', source_record: an_instance_of(Article), related_record: an_instance_of(User), authorizer: chained_authorizer)
+          allow_operation('include_has_many_resource', source_record: an_instance_of(Article), record_class: Comment, authorizer: chained_authorizer)
+        end
+
+        it { is_expected.to be_successful }
+
+        it 'includes only comments allowed by policy scope and associated with the article' do
+          json_comments = json_included.select { |item| item['type'] == 'comments' }
+          expect(json_comments.length).to eq(comments_policy_scope.length)
+          expect(
+            json_comments.map { |i| i['id'] }
+          ).to match_array(comments_policy_scope.pluck(:id).map(&:to_s))
+        end
+
+        it 'includes the associated author resource' do
+          json_users = json_included.select { |item| item['type'] == 'users' }
+          expect(json_users).to include(a_hash_including('id' => article.author.id.to_s))
+        end
+      end
+    end
+
+    describe 'a deep relationship' do
+      let(:include_query) { 'author.comments' }
+      let(:comments_policy_scope) { Comment.where(id: article.author.comments.first.id) }
+
+      context 'authorized for first relationship' do
+        before { allow_operation('include_has_one_resource', source_record: an_instance_of(Article), related_record: an_instance_of(User), authorizer: chained_authorizer) }
+
+        context 'authorized for second relationship' do
+          before { allow_operation('include_has_many_resource', source_record: an_instance_of(User), record_class: Comment, authorizer: chained_authorizer) }
+
+          it { is_expected.to be_successful }
+
+          it 'includes the first level resource' do
+            json_users = json_included.select { |item| item['type'] == 'users' }
+            expect(json_users).to include(a_hash_including('id' => article.author.id.to_s))
+          end
+
+          describe 'second level resources' do
+            it 'includes only resources allowed by policy scope' do
+              second_level_items = json_included.select { |item| item['type'] == 'comments' }
+              expect(second_level_items.length).to eq(comments_policy_scope.length)
+              expect(
+                second_level_items.map { |i| i['id'] }
+              ).to match_array(comments_policy_scope.pluck(:id).map(&:to_s))
+            end
+          end
+        end
+      end
+    end
+  end
+
+  shared_examples_for :scope_limited_directive_test_modify_relationships do
+    describe 'one-level deep has_many relationship' do
+      let(:comments_policy_scope) { Comment.where(id: existing_comments.first.id) }
+      let(:include_query) { 'comments' }
+
+      context 'authorized for include_has_many_resource for Comment' do
+        before {
+          allow_operation(
+            'include_has_many_resource',
+            source_record: an_instance_of(Article),
+            record_class: Comment,
+            authorizer: chained_authorizer
+          )
+        }
+
+        it { is_expected.to be_not_found }
+      end
+    end
+
+    describe 'multiple one-level deep relationships' do
+      let(:include_query) { 'author,comments' }
+      let(:comments_policy_scope) { Comment.where(id: existing_comments.first.id) }
+
+      context 'authorized for both operations' do
+        before do
+          allow_operation('include_has_one_resource', source_record: an_instance_of(Article), related_record: an_instance_of(User), authorizer: chained_authorizer)
+          allow_operation('include_has_many_resource', source_record: an_instance_of(Article), record_class: Comment, authorizer: chained_authorizer)
+        end
+
+        it { is_expected.to be_not_found }
+      end
+    end
+
+    describe 'a deep relationship' do
+      let(:include_query) { 'author.comments' }
+      let(:comments_policy_scope) { Comment.where(id: existing_author.comments.first.id) }
+
+      context 'authorized for first relationship' do
+        before { allow_operation('include_has_one_resource', source_record: an_instance_of(Article), related_record: an_instance_of(User), authorizer: chained_authorizer) }
+
+        context 'authorized for second relationship' do
+          before { allow_operation('include_has_many_resource', source_record: an_instance_of(User), record_class: Comment, authorizer: chained_authorizer) }
+
+          it { is_expected.to be_not_found }
+        end
+      end
+    end
+  end
+
   describe 'GET /articles' do
     subject(:last_response) { get("/articles?include=#{include_query}") }
     let!(:chained_authorizer) { allow_operation('find', source_class: Article) }
@@ -244,6 +373,7 @@
 
     # TODO: Test properly with multiple articles, not just one.
     include_examples :include_directive_tests
+    include_examples :scope_limited_directive_tests
   end
 
   describe 'GET /articles/:id' do
@@ -261,6 +391,7 @@
     let!(:chained_authorizer) { allow_operation('show', source_record: article) }
 
     include_examples :include_directive_tests
+    include_examples :scope_limited_directive_tests
   end
 
   describe 'PATCH /articles/:id' do
@@ -290,6 +421,7 @@
     let!(:chained_authorizer) { allow_operation('replace_fields', source_record: article, related_records_with_context: []) }
 
     include_examples :include_directive_tests
+    include_examples :scope_limited_directive_tests
 
     context 'the request has already failed validations' do
       let(:include_query) { 'author.comments' }
@@ -315,7 +447,7 @@
         {
           relation_type: :to_one,
           relation_name: :author,
-          records: existing_author
+          records: [existing_author]
         },
         {
           relation_type: :to_many,
@@ -325,7 +457,7 @@
           # down in the other specs.
           #
           # This is fine, because we test resource create relationships with specific matcher
-          records: kind_of(Array)
+          records: kind_of(Enumerable)
         }
       ]
     end
@@ -367,6 +499,7 @@
     end
 
     include_examples :include_directive_tests
+    include_examples :scope_limited_directive_test_modify_relationships
 
     context 'the request has already failed validations' do
       let(:include_query) { 'author.comments' }
@@ -395,6 +528,7 @@
     let!(:chained_authorizer) { allow_operation('show_related_resources', source_record: article, related_record_class: article.class) }
 
     include_examples :include_directive_tests
+    include_examples :scope_limited_directive_tests
   end
 
   describe 'GET /articles/:id/article' do
@@ -412,5 +546,6 @@
     let!(:chained_authorizer) { allow_operation('show_related_resource', source_record: article, related_record: article) }
 
     include_examples :include_directive_tests
+    include_examples :scope_limited_directive_tests
   end
 end

spec/requests/relationship_operations_spec.rb

@@ -423,12 +423,10 @@
       context 'limited by policy scope on comments' do
         let(:comments_scope) { Comment.none }
         before do
-          allow_operation('remove_to_many_relationship', source_record: article, related_records: [], relationship_type: :comments)
+          disallow_operation('remove_to_many_relationship', source_record: article, related_records: comments_to_remove, relationship_type: :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 }
+        it { is_expected.to be_not_found }
       end
 
       # If this happens in real life, it's mostly a bug. We want to document the