Merged in r2-2734-preview-allows-read-attachments (pull request #6666)

R2-2734 - A user who can preview records can see audio/images

Approved-by: Pavel Nabutovsky

app/models/ability.rb

@@ -168,15 +168,11 @@ def configure_exports
 
   def configure_record_attachments
     can(:read, Attachment) do |instance|
-      permitted_attachment_service = PermittedAttachmentService.new(user, instance, @permitted_form_fields_service)
-      permitted_attachment_service.permitted_to_access? || permitted_attachment_service.permitted_to_view? ||
-        permitted_attachment_service.permitted_to_preview?
+      PermittedAttachmentService.permitted_to_read?(user, instance, @permitted_form_fields_service)
     end
 
     can(%i[create destroy], Attachment) do |instance|
-      PermittedAttachmentService.new(
-        user, instance, @permitted_form_fields_service, true
-      ).permitted_to_access?
+      PermittedAttachmentService.permitted_to_write?(user, instance, @permitted_form_fields_service)
     end
   end
 

app/models/attachment.rb

@@ -84,6 +84,10 @@ def valid_content_types
     end
   end
 
+  def photo?
+    attachment_type == Attachment::IMAGE && field_name == Attachable::PHOTOS_FIELD_NAME
+  end
+
   def url
     Rails.application.routes.url_helpers.rails_blob_path(file, only_path: true, expires_in: EXPIRES,
                                                                disposition: :attachment)

app/services/permitted_attachment_service.rb

@@ -4,17 +4,31 @@
 
 # A service to determine if an attachment is permitted for a user
 class PermittedAttachmentService
+  PREVIEW_TYPES = [Attachment::IMAGE, Attachment::AUDIO].freeze
+
   attr_accessor :user, :write, :attachment, :permitted_form_fields_service
 
+  class << self
+    def permitted_to_read?(user, attachment, permitted_form_fields_service)
+      instance = new(user, attachment, permitted_form_fields_service)
+      instance.permitted? || instance.permitted_to_view? || instance.permitted_to_preview?
+    end
+
+    def permitted_to_write?(user, attachment, permitted_form_fields_service)
+      instance = new(user, attachment, permitted_form_fields_service, true)
+      instance.permitted?
+    end
+  end
+
   def initialize(user, attachment, permitted_form_fields_service, write = false)
     self.user = user
     self.attachment = attachment
     self.write = write
     self.permitted_form_fields_service = permitted_form_fields_service || PermittedFormFieldsService.instance
   end
 
-  def permitted_to_access?
-    permitted_to_access_record? && permitted_field_names.include?(attachment.field_name)
+  def permitted?
+    permitted_field_names.include?(attachment.field_name) && permitted_to_access_record?
   end
 
   def permitted_to_view?
@@ -33,20 +47,17 @@ def permitted_to_preview?
     # the fields displayed in the preview are those where show_on_minify_form: true.
     # Should we check the if the attachment field is permitted and if the field is show_on_minify_form: true?
 
-    user.can?(:search_owned_by_others, attachment.record.class) && (
-      permitted_to_preview_attachment? || permitted_to_view_record_list_photo?
+    previewable_type? && user.can?(:search_owned_by_others, attachment.record.class) && (
+      user.can_preview?(attachment.record.class) || permitted_to_view_record_list_photo?
     )
   end
 
-  def permitted_to_preview_attachment?
-    user.can_preview?(attachment.record.class) &&
-      [Attachment::IMAGE, Attachment::AUDIO].include?(attachment.attachment_type) &&
-      permitted_field_names.include?(attachment.field_name)
+  def permitted_to_view_record_list_photo?
+    attachment.photo? && user.can?(:view_photo, attachment.record.class)
   end
 
-  def permitted_to_view_record_list_photo?
-    attachment.attachment_type == Attachment::IMAGE && user.can?(:view_photo, attachment.record.class) &&
-      attachment.field_name == Attachable::PHOTOS_FIELD_NAME
+  def previewable_type?
+    PREVIEW_TYPES.include?(attachment.attachment_type)
   end
 
   def permitted_field_names

app/services/permitted_field_service.rb

@@ -111,6 +111,7 @@ def permitted_field_names(writeable = false, update = false, roles = [])
     @permitted_field_names += permitted_reporting_location_field
     @permitted_field_names += permitted_registry_record_id
     @permitted_field_names += permitted_family_id
+    @permitted_field_names += permitted_attachment_fields
     @permitted_field_names
   end
 
@@ -223,5 +224,17 @@ def permitted_mrm_entities_schema
       schema[entry] = { 'type' => %w[array null], 'items' => { 'type' => 'object' } }
     end
   end
+
+  def permitted_attachment_fields
+    attachment_field_names = []
+    if user.can?(:search_owned_by_others, model_class) && user.can_preview?(model_class)
+      attachment_field_names << Attachable::PHOTOS_FIELD_NAME
+      attachment_field_names << Attachable::AUDIOS_FIELD_NAME
+    end
+
+    attachment_field_names << 'photo' if user.can?(:view_photo, model_class)
+
+    attachment_field_names
+  end
 end
 # rubocop:enable Metrics/ClassLength

app/services/record_data_service.rb

@@ -20,7 +20,7 @@ def self.visible_name(record)
   def data(record, user, selected_field_names)
     data = select_fields(record.data, selected_field_names)
     data = embebed_data(data, record, selected_field_names, user)
-    data = embed_photo_metadata(data, record, selected_field_names, user)
+    data = embed_photo_metadata(data, record, selected_field_names)
     data = embed_attachments(data, record, selected_field_names)
     data = embed_associations_as_data(data, record, selected_field_names, user)
     data['last_updated_at'] = record.last_updated_at
@@ -64,8 +64,8 @@ def embed_flag_metadata(data, record, selected_field_names)
     data
   end
 
-  def embed_photo_metadata(data, record, selected_field_names, user)
-    return data unless selected_field_names.include?('photos') || user.can?(:view_photo, record.class)
+  def embed_photo_metadata(data, record, selected_field_names)
+    return data unless (selected_field_names & %w[photos photo]).present?
 
     data['photo'] = record.photo_url
     data

spec/models/ability_spec.rb

@@ -1099,6 +1099,8 @@
 
     let(:preview_user) { create :user }
 
+    let(:child_other) { create :child }
+
     let(:child_with_attachment) do
       child_with_attachment = Child.new_with_user(
         attachment_user,
@@ -1107,6 +1109,27 @@
       child_with_attachment.save! && child_with_attachment
     end
 
+    let(:image_attachment_other) do
+      Attachment.new(
+        record: child_other, field_name: Attachable::PHOTOS_FIELD_NAME, attachment_type: Attachment::IMAGE,
+        file_name: 'jorge.jpg', attachment: attachment_base64('jorge.jpg')
+      )
+    end
+
+    let(:audio_attachment_other) do
+      Attachment.new(
+        record: child_other, field_name: Attachable::AUDIOS_FIELD_NAME, attachment_type: Attachment::AUDIO,
+        file_name: 'sample.mp3', attachment: attachment_base64('sample.mp3')
+      )
+    end
+
+    let(:document_attachment_other) do
+      Attachment.new(
+        record: child_other, field_name: Attachable::DOCUMENTS_FIELD_NAME, file_name: 'dummy.pdf',
+        attachment_type: Attachment::DOCUMENT, attachment: attachment_base64('dummy.pdf')
+      )
+    end
+
     let(:image_attachment) do
       Attachment.new(
         record: child_with_attachment, field_name: Attachable::PHOTOS_FIELD_NAME, attachment_type: Attachment::IMAGE,
@@ -1326,137 +1349,112 @@
       end
     end
 
-    context 'when a user can preview a record' do
-      context 'and has access to attachment fields' do
-        before do
-          permission = Permission.new(resource: Permission::CASE, actions:
-            [
-              Permission::DISPLAY_VIEW_PAGE, Permission::SEARCH_OWNED_BY_OTHERS
-            ])
-          role = Role.new_with_properties(
-            unique_id: 'preview_access_attachment', name: 'preview_access_attachment', permissions: [permission],
-            form_section_read_write: { form1: 'rw' }
-          )
-          role.save!
-          preview_user.role = role
-          preview_user.save!
-        end
-
-        it 'allows viewing attached images/audios' do
-          ability = Ability.new preview_user
-
-          expect(ability).to authorize(:read, image_attachment)
-          expect(ability).to authorize(:read, audio_attachment)
-        end
-
-        it 'does not allow viewing attached documents' do
-          ability = Ability.new preview_user
-          expect(ability).not_to authorize(:read, document_attachment)
-        end
-
-        it 'does not allow deleting attachments' do
-          ability = Ability.new preview_user
-          expect(ability).not_to authorize(:destroy, image_attachment)
-          expect(ability).not_to authorize(:destroy, audio_attachment)
-          expect(ability).not_to authorize(:destroy, document_attachment)
-        end
-      end
-
-      context 'and does not have access to attachment fields' do
-        before do
-          permission = Permission.new(resource: Permission::CASE, actions:
-            [
-              Permission::DISPLAY_VIEW_PAGE, Permission::SEARCH_OWNED_BY_OTHERS
-            ])
-          role = Role.new_with_properties(
-            unique_id: 'preview_record', name: 'preview_record', permissions: [permission]
-          )
-          role.save!
-          preview_user.role = role
-          preview_user.save!
-        end
-
-        it 'does not allow viewing attachments' do
-          ability = Ability.new preview_user
-          expect(ability).not_to authorize(:read, image_attachment)
-          expect(ability).not_to authorize(:read, audio_attachment)
-          expect(ability).not_to authorize(:read, document_attachment)
-        end
+    context 'when a user does not have access to a record' do
+      context 'and a user can search records owned by others' do
+        context 'and can preview a record' do
+          context 'and has access to attachment fields' do
+            before do
+              permission = Permission.new(resource: Permission::CASE, actions:
+                [
+                  Permission::DISPLAY_VIEW_PAGE, Permission::SEARCH_OWNED_BY_OTHERS
+                ])
+              role = Role.new_with_properties(
+                unique_id: 'preview_access_attachment', name: 'preview_access_attachment', permissions: [permission],
+                form_section_read_write: { form1: 'rw' }
+              )
+              role.save!
+              preview_user.role = role
+              preview_user.save!
+            end
+
+            it 'allows viewing attached images/audios' do
+              ability = Ability.new preview_user
+
+              expect(ability).to authorize(:read, image_attachment_other)
+              expect(ability).to authorize(:read, audio_attachment_other)
+            end
+
+            it 'does not allow viewing attached documents' do
+              ability = Ability.new preview_user
+              expect(ability).not_to authorize(:read, document_attachment_other)
+            end
+
+            it 'does not allow deleting attachments' do
+              ability = Ability.new preview_user
+              expect(ability).not_to authorize(:destroy, image_attachment_other)
+              expect(ability).not_to authorize(:destroy, audio_attachment_other)
+              expect(ability).not_to authorize(:destroy, document_attachment_other)
+            end
+          end
 
-        it 'does not allow deleting attachments' do
-          ability = Ability.new preview_user
-          expect(ability).not_to authorize(:destroy, image_attachment)
-          expect(ability).not_to authorize(:destroy, audio_attachment)
-          expect(ability).not_to authorize(:destroy, document_attachment)
+          context 'and does not have access to attachment fields' do
+            before do
+              permission = Permission.new(resource: Permission::CASE, actions:
+                [
+                  Permission::DISPLAY_VIEW_PAGE, Permission::SEARCH_OWNED_BY_OTHERS
+                ])
+              role = Role.new_with_properties(
+                unique_id: 'preview_record', name: 'preview_record', permissions: [permission]
+              )
+              role.save!
+              preview_user.role = role
+              preview_user.save!
+            end
+
+            it 'allows viewing image/audio attachments' do
+              ability = Ability.new preview_user
+              expect(ability).to authorize(:read, image_attachment_other)
+              expect(ability).to authorize(:read, audio_attachment_other)
+            end
+
+            it 'does not allow viewing document attachment' do
+              ability = Ability.new preview_user
+              expect(ability).not_to authorize(:read, document_attachment_other)
+            end
+
+            it 'does not allow deleting attachments' do
+              ability = Ability.new preview_user
+              expect(ability).not_to authorize(:destroy, image_attachment_other)
+              expect(ability).not_to authorize(:destroy, audio_attachment_other)
+              expect(ability).not_to authorize(:destroy, document_attachment_other)
+            end
+          end
         end
 
-        context 'and is permitted to view photo from the list view' do
+        context 'and can view a photo from the list view' do
           before do
-            permission = Permission.new(resource: Permission::CASE, actions:
-              [
-                Permission::DISPLAY_VIEW_PAGE, Permission::SEARCH_OWNED_BY_OTHERS, Permission::VIEW_PHOTO
-              ])
+            permission = Permission.new(
+              resource: Permission::CASE, actions: [Permission::SEARCH_OWNED_BY_OTHERS, Permission::VIEW_PHOTO]
+            )
             role = Role.new_with_properties(
-              unique_id: 'preview_photo_record', name: 'preview_photo_record', permissions: [permission],
-              form_section_read_write: { form2: 'r' }
+              unique_id: 'view_photo_attachment', name: 'view_photo_attachment', permissions: [permission]
             )
             role.save!
-            preview_user.role = role
-            preview_user.save!
+            attachment_user.role = role
+            attachment_user.save!
           end
 
-          it 'allows viewing images' do
-            ability = Ability.new preview_user
-            expect(ability).to authorize(:read, image_attachment)
+          it 'allows viewing attached images' do
+            ability = Ability.new attachment_user
+            expect(ability).to authorize(:read, image_attachment_other)
           end
 
-          it 'does not allow viewing audios/documents' do
-            ability = Ability.new preview_user
-            expect(ability).not_to authorize(:read, audio_attachment)
-            expect(ability).not_to authorize(:read, document_attachment)
+          it 'does not allow viewing attached audios/documents' do
+            ability = Ability.new attachment_user
+            expect(ability).not_to authorize(:read, audio_attachment_other)
+            expect(ability).not_to authorize(:read, document_attachment_other)
           end
 
           it 'does not allow deleting attachments' do
-            ability = Ability.new preview_user
-            expect(ability).not_to authorize(:destroy, image_attachment)
-            expect(ability).not_to authorize(:destroy, audio_attachment)
-            expect(ability).not_to authorize(:destroy, document_attachment)
+            ability = Ability.new attachment_user
+            expect(ability).not_to authorize(:destroy, image_attachment_other)
+            expect(ability).not_to authorize(:destroy, audio_attachment_other)
+            expect(ability).not_to authorize(:destroy, document_attachment_other)
           end
         end
       end
     end
 
-    context 'when a user can view a photo from the list view' do
-      before do
-        permission = Permission.new(resource: Permission::CASE, actions: [Permission::VIEW_PHOTO])
-        role = Role.new_with_properties(
-          unique_id: 'view_photo_attachment', name: 'view_photo_attachment', permissions: [permission],
-          form_section_read_write: { form2: 'r' }
-        )
-        role.save!
-        attachment_user.role = role
-        attachment_user.save!
-      end
-
-      it 'allows viewing attached images' do
-        ability = Ability.new attachment_user
-        expect(ability).to authorize(:read, image_attachment)
-      end
-
-      it 'does not allow viewing attached audios/documents' do
-        ability = Ability.new attachment_user
-        expect(ability).not_to authorize(:read, audio_attachment)
-        expect(ability).not_to authorize(:read, document_attachment)
-      end
-
-      it 'does not allow deleting attachments' do
-        ability = Ability.new attachment_user
-        expect(ability).not_to authorize(:destroy, image_attachment)
-        expect(ability).not_to authorize(:destroy, audio_attachment)
-        expect(ability).not_to authorize(:destroy, document_attachment)
-      end
-    end
-
     context 'when read/write access is not permitted' do
       before do
         permission = Permission.new(resource: Permission::CASE, actions: [Permission::READ])