From e097036296e47f865ab181e0623d448f2a5747fa Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 13 Jul 2023 10:51:10 +0200
Subject: [PATCH] Fix RfC Visibility to work as intended

The RfC Visibility of a consumer is intended to restrict who can access which RfCs. So far, those restrictions were only applied one way, for learners of a restricted consumer to view other (external) RfCs. However, the other way around should also work: If a RfC was created as part of a restricted consumer, no other external user should be able to interfere with this RfC. This commit, therefore, adds this direction as well and covers both directions with tests.
---
 app/policies/request_for_comment_policy.rb    |  29 +-
 spec/factories/request_for_comment.rb         |   2 +-
 .../request_for_comment_policy_spec.rb        | 275 ++++++++++++------
 3 files changed, 207 insertions(+), 99 deletions(-)

diff --git a/app/policies/request_for_comment_policy.rb b/app/policies/request_for_comment_policy.rb
index 160fbc15..a5e863e2 100644
--- a/app/policies/request_for_comment_policy.rb
+++ b/app/policies/request_for_comment_policy.rb
@@ -6,7 +6,7 @@ class RequestForCommentPolicy < ApplicationPolicy
   end
 
   def show?
-    admin? || rfc_visibility
+    admin? || author? || rfc_visibility
   end
 
   def destroy?
@@ -14,11 +14,11 @@ class RequestForCommentPolicy < ApplicationPolicy
   end
 
   def mark_as_solved?
-    admin? || (author? && rfc_visibility)
+    admin? || author?
   end
 
   def set_thank_you_note?
-    admin? || (author? && rfc_visibility)
+    admin? || author?
   end
 
   def clear_question?
@@ -42,12 +42,16 @@ class RequestForCommentPolicy < ApplicationPolicy
   end
 
   def rfc_visibility
-    case @user.consumer.rfc_visibility
-      when 'all'
+    # The consumer with the most restricted visibility determines the visibility of the RfC
+    case [@user.consumer.rfc_visibility, @record.author.consumer.rfc_visibility]
+      # Only if both consumers allow learners to see all RfCs, the RfC is visible to the learner
+      when %w[all all]
         everyone
-      when 'consumer'
+      # At least one consumer limits the visibility to the consumer
+      when %w[consumer all], %w[all consumer], %w[consumer consumer]
         @record.author.consumer == @user.consumer
-      when 'study_group'
+      # At least one consumer limits the visibility to the study group
+      when %w[study_group all], %w[all study_group], %w[study_group consumer], %w[consumer study_group], %w[study_group study_group]
         @record.submission.study_group.present? && @record.submission.study_group.id == @user.current_study_group_id
       else
         raise "Unknown RfC Visibility #{current_user.consumer.rfc_visibility}"
@@ -61,8 +65,16 @@ class RequestForCommentPolicy < ApplicationPolicy
       else
         case @user.consumer.rfc_visibility
           when 'all'
-            @scope.all
+            # We need to filter those RfCs where the visibility is more restricted than the `all` visibility.
+            rfcs_with_users = @scope
+              .joins('LEFT OUTER JOIN external_users ON request_for_comments.user_type = \'ExternalUser\' AND request_for_comments.user_id = external_users.id')
+              .joins('LEFT OUTER JOIN internal_users ON request_for_comments.user_type = \'InternalUser\' AND request_for_comments.user_id = internal_users.id')
+
+            rfcs_with_users.where(external_users: {consumer_id: Consumer.rfc_visibility_all})
+              .or(rfcs_with_users.where(internal_users: {consumer_id: Consumer.rfc_visibility_all}))
           when 'consumer'
+            # Since the `rfc_visibility` is set on a consumer level, we do not need to consider the `study_group` visibility here.
+            # Therefore, those RfCs where the author is limited to study group RfCs definitely belong to another consumer.
             rfcs_with_users = @scope
               .joins('LEFT OUTER JOIN external_users ON request_for_comments.user_type = \'ExternalUser\' AND request_for_comments.user_id = external_users.id')
               .joins('LEFT OUTER JOIN internal_users ON request_for_comments.user_type = \'InternalUser\' AND request_for_comments.user_id = internal_users.id')
@@ -70,6 +82,7 @@ class RequestForCommentPolicy < ApplicationPolicy
             rfcs_with_users.where(external_users: {consumer_id: @user.consumer.id})
               .or(rfcs_with_users.where(internal_users: {consumer_id: @user.consumer.id}))
           when 'study_group'
+            # Since the `rfc_visibility` is already the most restricted visibility, we do not need to consider any other visibility here.
             @scope
               .joins(:submission)
               .where(submission: {study_group: @user.current_study_group_id})
diff --git a/spec/factories/request_for_comment.rb b/spec/factories/request_for_comment.rb
index b6dc4ed2..fb595e36 100644
--- a/spec/factories/request_for_comment.rb
+++ b/spec/factories/request_for_comment.rb
@@ -4,7 +4,7 @@ FactoryBot.define do
   factory :rfc, class: 'RequestForComment' do
     user factory: :external_user
     exercise factory: :math
-    submission { association :submission, exercise:, user: }
+    submission { association :submission, exercise:, user:, study_group: user&.study_groups&.first }
     file
     sequence :question do |n|
       "test question #{n}"
diff --git a/spec/policies/request_for_comment_policy_spec.rb b/spec/policies/request_for_comment_policy_spec.rb
index 184cc4c3..ab883637 100644
--- a/spec/policies/request_for_comment_policy_spec.rb
+++ b/spec/policies/request_for_comment_policy_spec.rb
@@ -49,150 +49,245 @@ describe RequestForCommentPolicy do
   end
 
   context 'when the RfC visibility is considered' do
-    let(:user) { create(:learner, consumer:) }
+    shared_examples 'grants access to everyone' do
+      it 'grants access to everyone' do
+        %i[external_user teacher admin].each do |factory_name|
+          expect(policy).to permit(create(factory_name, consumer: viewer_consumer, study_groups: viewer_study_groups), rfc)
+        end
+      end
 
-    let(:rfc_other_consumer) do
-      rfc = create(:rfc, user:)
-      rfc.user.update(consumer: create(:consumer, name: 'Other Consumer'))
-      rfc
+      it 'grants access to authors' do
+        expect(policy).to permit(rfc.author, rfc)
+      end
     end
 
-    let(:rfc_other_study_group) do
-      rfc = create(:rfc, user:)
-      rfc.user.update(consumer: user.consumer)
-      rfc.submission.update(study_group: create(:study_group))
-      rfc
+    shared_examples 'grants access to admins and authors only' do
+      it 'grants access to admins' do
+        expect(policy).to permit(create(:admin, consumer: viewer_consumer, study_groups: viewer_study_groups), rfc)
+      end
+
+      it 'grants access to authors' do
+        expect(policy).to permit(rfc.author, rfc)
+      end
+
+      it 'does not grant access to all other users' do
+        %i[external_user teacher].each do |factory_name|
+          expect(policy).not_to permit(create(factory_name, consumer: viewer_consumer, study_groups: viewer_study_groups), rfc)
+        end
+      end
     end
 
-    let(:rfc_peer) do
-      rfc = create(:rfc, user:)
-      rfc.user.update(consumer: user.consumer)
-      rfc.submission.update(study_group: user.study_groups.first)
-      rfc
-    end
+    let(:rfc_author) { create(:learner, consumer: author_consumer, study_groups: author_study_groups) }
+    let(:author_study_groups) { create_list(:study_group, 1, consumer: author_consumer) }
+    let(:rfc) { create(:rfc, user: rfc_author) }
 
-    let(:all_rfcs) { [rfc_other_consumer, rfc_other_study_group, rfc_peer] }
-    let(:same_consumer_rfcs) { [rfc_other_study_group, rfc_peer] }
-    let(:other_study_groups_rfcs) { [rfc_other_consumer, rfc_other_study_group] }
+    context "when the author's rfc_visibility is set to all" do
+      let(:author_consumer) { create(:consumer, rfc_visibility: 'all') }
 
-    context 'when rfc_visibility is set to all' do
-      let(:consumer) { create(:consumer, rfc_visibility: 'all') }
+      context 'when the viewer is from another consumer' do
+        context "when the viewer's rfc_visibility is set to all" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'all') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
 
-      permissions(:show?) do
-        it 'grants access to everyone' do
-          %i[external_user teacher admin].each do |factory_name|
-            all_rfcs.each do |specific_rfc|
-              expect(policy).to permit(create(factory_name, consumer:), specific_rfc)
+          permissions(:show?) do
+            include_examples 'grants access to everyone'
+          end
+
+          %i[mark_as_solved? set_thank_you_note?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
+            end
+          end
+        end
+
+        context "when the viewer's rfc_visibility is set to consumer" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'consumer') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
+            end
+          end
+        end
+
+        context "when the viewer's rfc_visibility is set to study_group" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'study_group') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
             end
           end
         end
       end
 
-      %i[mark_as_solved? set_thank_you_note?].each do |action|
-        permissions(action) do
-          it 'grants access to admins' do
-            all_rfcs.each do |specific_rfc|
-              expect(policy).to permit(create(:admin, consumer:), specific_rfc)
-            end
+      context 'when the viewer is from the same consumer' do
+        let(:viewer_consumer) { author_consumer }
+
+        context 'when the viewer is from another study group' do
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          permissions(:show?) do
+            include_examples 'grants access to everyone'
           end
 
-          it 'grants access to authors' do
-            all_rfcs.each do |specific_rfc|
-              expect(policy).to permit(specific_rfc.author, specific_rfc)
+          %i[mark_as_solved? set_thank_you_note?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
             end
           end
+        end
 
-          it 'does not grant access to all other users' do
-            %i[external_user teacher].each do |factory_name|
-              all_rfcs.each do |specific_rfc|
-                expect(policy).not_to permit(create(factory_name, consumer:), specific_rfc)
-              end
+        context 'when the viewer is from the same study group' do
+          let(:viewer_study_groups) { author_study_groups }
+
+          permissions(:show?) do
+            include_examples 'grants access to everyone'
+          end
+
+          %i[mark_as_solved? set_thank_you_note?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
             end
           end
         end
       end
     end
 
-    context 'when rfc_visibility is set to consumer' do
-      let(:consumer) { create(:consumer, rfc_visibility: 'consumer') }
+    context "when the author's rfc_visibility is set to consumer" do
+      let(:author_consumer) { create(:consumer, rfc_visibility: 'consumer') }
 
-      permissions(:show?) do
-        it 'grants access to admins' do
-          all_rfcs.each do |specific_rfc|
-            expect(policy).to permit(create(:admin, consumer:), specific_rfc)
-          end
-        end
+      context 'when the viewer is from another consumer' do
+        context "when the viewer's rfc_visibility is set to all" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'all') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
 
-        it 'grants access to users from the same consumer' do
-          %i[external_user teacher].each do |factory_name|
-            same_consumer_rfcs.each do |specific_rfc|
-              expect(policy).to permit(create(factory_name, consumer:), specific_rfc)
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
             end
           end
         end
 
-        it 'does not grant access to users from other consumers' do
-          %i[external_user teacher].each do |factory_name|
-            expect(policy).not_to permit(create(factory_name, consumer:), rfc_other_consumer)
+        context "when the viewer's rfc_visibility is set to consumer" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'consumer') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
+            end
+          end
+        end
+
+        context "when the viewer's rfc_visibility is set to study_group" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'study_group') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
+            end
           end
         end
       end
 
-      # Testing `mark_as_solved?` and `set_thank_you_note?` is not necessary here,
-      # because an author of an RfC can only be a user from the same consumer.
+      context 'when the viewer is from the same consumer' do
+        let(:viewer_consumer) { author_consumer }
+
+        context 'when the viewer is from another study group' do
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          permissions(:show?) do
+            include_examples 'grants access to everyone'
+          end
+
+          %i[mark_as_solved? set_thank_you_note?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
+            end
+          end
+        end
+
+        context 'when the viewer is from the same study group' do
+          let(:viewer_study_groups) { author_study_groups }
+
+          permissions(:show?) do
+            include_examples 'grants access to everyone'
+          end
+
+          %i[mark_as_solved? set_thank_you_note?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
+            end
+          end
+        end
+      end
     end
 
-    context 'when rfc_visibility is set to study_group' do
-      let(:consumer) { create(:consumer, rfc_visibility: 'study_group') }
+    context "when the author's rfc_visibility is set to study_group" do
+      let(:author_consumer) { create(:consumer, rfc_visibility: 'study_group') }
 
-      permissions(:show?) do
-        it 'grants access to admins' do
-          all_rfcs.each do |specific_rfc|
-            expect(policy).to permit(create(:admin, consumer:), specific_rfc)
+      context 'when the viewer is from another consumer' do
+        context "when the viewer's rfc_visibility is set to all" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'all') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
+            end
           end
         end
 
-        it 'grants access to users from the same study group' do
-          %i[external_user teacher].each do |factory_name|
-            expect(policy).to permit(create(factory_name, consumer:, study_groups: [rfc_peer.submission.study_group]), rfc_peer)
+        context "when the viewer's rfc_visibility is set to consumer" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'consumer') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
+            end
           end
         end
 
-        it 'does not grant access to users from other consumers' do
-          %i[external_user teacher].each do |factory_name|
-            other_study_groups_rfcs.each do |specific_rfc|
-              expect(policy).not_to permit(create(factory_name, consumer:), specific_rfc)
+        context "when the viewer's rfc_visibility is set to study_group" do
+          let(:viewer_consumer) { create(:consumer, name: 'Other Consumer', rfc_visibility: 'study_group') }
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
             end
           end
         end
       end
 
-      %i[mark_as_solved? set_thank_you_note?].each do |action|
-        permissions(action) do
-          it 'grants access to admins' do
-            all_rfcs.each do |specific_rfc|
-              expect(policy).to permit(create(:admin, consumer:), specific_rfc)
+      context 'when the viewer is from the same consumer' do
+        let(:viewer_consumer) { author_consumer }
+
+        context 'when the viewer is from another study group' do
+          let(:viewer_study_groups) { create_list(:study_group, 1, consumer: viewer_consumer) }
+
+          %i[mark_as_solved? set_thank_you_note? show?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
             end
           end
+        end
 
-          # Testing `mark_as_solved?` and `set_thank_you_note?` with another consumer is not
-          # necessary here, because an author of an RfC can only be a user from the same consumer.
+        context 'when the viewer is from the same study group' do
+          let(:viewer_study_groups) { author_study_groups }
 
-          it 'grants access to authors of the same primary study group' do
-            rfc_peer.author.update(study_groups: [rfc_peer.submission.study_group])
-            expect(policy).to permit(rfc_peer.author, rfc_peer)
+          permissions(:show?) do
+            include_examples 'grants access to everyone'
           end
 
-          it 'does not grant access to authors of another primary study groups' do
-            rfc_other_study_group.author.update(study_groups: create_list(:study_group, 1))
-            expect(policy).not_to permit(rfc_other_study_group.author, rfc_other_study_group)
-          end
-
-          it 'does not grant access to all other users' do
-            %i[external_user teacher].each do |factory_name|
-              all_rfcs.each do |specific_rfc|
-                expect(policy).not_to permit(create(factory_name, consumer:), specific_rfc)
-              end
+          %i[mark_as_solved? set_thank_you_note?].each do |action|
+            permissions(action) do
+              include_examples 'grants access to admins and authors only'
             end
           end
         end