[ActiveStorage for All] Unify staged upload params behind an ownership-enforcing resolver

app/controllers/concerns/hyrax/enforces_staged_upload_ownership.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module Hyrax
+  ##
+  # Rescues {Hyrax::UploadedFileResolver::OwnershipError}, raised when a
+  # request tries to attach staged uploads belonging to another user.
+  module EnforcesStagedUploadOwnership
+    extend ActiveSupport::Concern
+
+    included do
+      rescue_from Hyrax::UploadedFileResolver::OwnershipError,
+                  with: :render_staged_upload_ownership_error
+    end
+
+    private
+
+    def render_staged_upload_ownership_error(error)
+      message = I18n.t('hyrax.uploads.ownership_error')
+      Hyrax.logger.error(error.message)
+
+      respond_to do |wants|
+        wants.html { redirect_back fallback_location: main_app.root_path, alert: message }
+        wants.json { render json: { message: message }, status: :forbidden }
+      end
+    end
+  end
+end

app/controllers/concerns/hyrax/works_controller_behavior.rb

@@ -10,6 +10,7 @@ module WorksControllerBehavior
     include Hyrax::FlexibleSchemaBehavior if Hyrax.config.flexible?
     include Hyrax::EnsureMigratedBehavior
     include Hyrax::RedirectToDisplayUrl
+    include Hyrax::EnforcesStagedUploadOwnership
 
     included do
       with_themed_layout :decide_layout
@@ -550,7 +551,7 @@ def concern_has_file_sets?
     end
 
     def uploaded_files
-      UploadedFile.find(params.fetch(:uploaded_files, []))
+      Hyrax::UploadedFileResolver.call(params.fetch(:uploaded_files, []), user: current_user)
     end
 
     def available_admin_sets

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -8,6 +8,7 @@ class CollectionsController < Hyrax::My::CollectionsController
       include Blacklight::SearchContext
       include Hyrax::FlexibleSchemaBehavior if Hyrax.config.collection_flexible?
       include Hyrax::EnsureMigratedBehavior
+      include Hyrax::EnforcesStagedUploadOwnership
 
       configure_blacklight do |config|
         config.search_builder_class = Hyrax::Dashboard::CollectionsSearchBuilder
@@ -18,6 +19,7 @@ class CollectionsController < Hyrax::My::CollectionsController
 
       before_action :filter_docs_with_read_access!, except: [:show, :edit]
       before_action :remove_select_something_first_flash, except: :show
+      before_action :validate_branding_upload_ownership!, only: [:create, :update]
 
       include Hyrax::Collections::AcceptsBatches
 
@@ -308,6 +310,18 @@ def update_referer
         dashboard_collection_path(@collection)
       end
 
+      # Branding uploads arrive as staged Hyrax::UploadedFile ids in
+      # banner_files/logo_files (logo_files mixes new upload ids with local
+      # paths of already-attached logos). Both the ActiveFedora and the
+      # Valkyrie branding paths flow through this controller, so ownership is
+      # enforced here, at the parameter edge.
+      def validate_branding_upload_ownership!
+        Hyrax::UploadedFileResolver.call(params["banner_files"], user: current_user) unless
+          params["banner_unchanged"] == "true"
+        new_logo_ids = Array.wrap(params["logo_files"]).select { |ufi| ufi.to_s.match(/\D/).nil? }
+        Hyrax::UploadedFileResolver.call(new_logo_ids, user: current_user)
+      end
+
       def process_banner_input
         return update_existing_banner if params["banner_unchanged"] == "true"
         remove_banner

app/controllers/hyrax/file_sets_controller.rb

@@ -9,6 +9,7 @@ class FileSetsController < ApplicationController
     include Hyrax::Breadcrumbs
     include Hyrax::FlexibleSchemaBehavior if Hyrax.config.file_set_flexible?
     include Hyrax::EnsureMigratedBehavior
+    include Hyrax::EnforcesStagedUploadOwnership
 
     before_action :authenticate_user!, except: [:show, :citation, :stats]
     load_and_authorize_resource class: Hyrax.config.file_set_class
@@ -209,9 +210,8 @@ def attempt_update
         else
           update_metadata
         end
-      elsif params.key?(:files_files) # version file already uploaded with ref id in :files_files array
-        uploaded_files = Array(Hyrax::UploadedFile.find(params[:files_files]))
-        actor.update_content(uploaded_files.first)
+      elsif staged_version_upload? # version file already staged, its id in the params
+        actor.update_content(staged_version_upload)
         update_metadata
       end
     end
@@ -224,15 +224,35 @@ def attempt_update_valkyrie
         else
           update_metadata
         end
-      elsif params.key?(:files_files) # version file already uploaded with ref id in :files_files array
-        uploaded_files = Array(Hyrax::UploadedFile.find(params[:files_files]))
-        uploaded_files.first.file_set_uri = file_set.id.to_s
-        uploaded_files.first.save
-        ValkyrieIngestJob.perform_later(uploaded_files.first)
+      elsif staged_version_upload? # version file already staged, its id in the params
+        uploaded_file = staged_version_upload
+        uploaded_file.file_set_uri = file_set.id.to_s
+        uploaded_file.save
+        ValkyrieIngestJob.perform_later(uploaded_file)
         update_metadata
       end
     end
 
+    def staged_version_upload?
+      params.key?(:uploaded_files) || params.key?(:files_files)
+    end
+
+    # The staged upload submitted for a version update. +uploaded_files+ is
+    # the canonical field name shared with the work form; +files_files+ (the
+    # historical name generated by the versioning upload widget) is accepted
+    # as a deprecated alias.
+    #
+    # @return [Hyrax::UploadedFile, nil]
+    def staged_version_upload
+      ids = params[:uploaded_files]
+      if ids.blank? && params.key?(:files_files)
+        Deprecation.warn("The files_files parameter is deprecated and will be removed; " \
+                         "submit version uploads as uploaded_files instead.")
+        ids = params[:files_files]
+      end
+      Hyrax::UploadedFileResolver.call(ids, user: current_user).first
+    end
+
     def revert_valkyrie
       Hyrax::VersioningService.create(file_metadata, current_user, Hyrax.storage_adapter.find_by(id: params[:revision]))
       # update_metadata

app/services/hyrax/action/create_valkyrie_work.rb

@@ -73,7 +73,7 @@ def step_args
 
       # rubocop:disable Lint/DuplicateMethods
       def uploaded_files
-        UploadedFile.find(params.fetch(:uploaded_files, []))
+        Hyrax::UploadedFileResolver.call(params.fetch(:uploaded_files, []), user: user)
       end
       # rubocop:enable Lint/DuplicateMethods
     end

app/services/hyrax/uploaded_file_resolver.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+module Hyrax
+  ##
+  # @api public
+  #
+  # The single seam turning staged-upload id params into
+  # {Hyrax::UploadedFile} records, enforcing that the acting user owns every
+  # file being attached.
+  #
+  # The non-ActiveFedora attach paths (work create/update, file set version
+  # upload, collection branding) resolve their uploaded file params through
+  # this service. The ActiveFedora actor stack performs its equivalent check
+  # in {Hyrax::Actors::CreateWithFilesActor}.
+  #
+  # @example
+  #   Hyrax::UploadedFileResolver.call(params[:uploaded_files], user: current_user)
+  class UploadedFileResolver
+    ##
+    # Raised when staged uploads are attached by a user other than their
+    # owner. Rescued at the controller edge by
+    # {Hyrax::EnforcesStagedUploadOwnership}.
+    class OwnershipError < RuntimeError
+      def initialize(msg = "attempted to attach uploaded files owned by another user")
+        super
+      end
+    end
+
+    ##
+    # @param ids [Enumerable<String, Integer>, String, Integer, nil]
+    #   staged upload ids, as they arrive in params
+    # @param user [::User] the acting user
+    #
+    # @return [Array<Hyrax::UploadedFile>] the resolved files, in id order
+    #
+    # @raise [ActiveRecord::RecordNotFound] when an id does not exist
+    # @raise [OwnershipError] when a file belongs to another user
+    def self.call(ids, user:)
+      ids = Array.wrap(ids).select(&:present?)
+      return [] if ids.empty?
+
+      files = Array.wrap(Hyrax::UploadedFile.find(ids))
+      foreign = files.reject { |file| file.user_id == user&.id }
+
+      foreign.each do |file|
+        Hyrax.logger.error "User #{user.try(:user_key)} attempted to ingest uploaded_file #{file.id}, " \
+                           "but it belongs to a different user"
+      end
+      raise OwnershipError unless foreign.empty?
+
+      files
+    end
+  end
+end

config/locales/hyrax.de.yml

@@ -1741,6 +1741,7 @@ de:
       permissions_message: Aktualisieren von Dateiberechtigungen. Dieser Vorgang kann ein paar Minuten dauern. Sie können Ihren Browser aktualisieren oder später zu diesem Datensatz zurückkehren, um die aktualisierten Dateiberechtigungen zu sehen.
       processing: Datei wird verarbeitet. Sie können weiter bearbeiten, wenn dieser Vorgang abgeschlossen ist
     uploads:
+      ownership_error: "Hochgeladene Dateien konnten nicht angehängt werden, da sie einem anderen Benutzer gehören."
       js_templates:
         display_label: Beschriftung anzeigen
         error: Fehler

config/locales/hyrax.en.yml

@@ -1873,6 +1873,7 @@ en:
       permissions_message: Updating file permissions. This may take a few minutes. You may want to refresh your browser or return to this record later to see the updated file permissions.
       processing: File is being processed; you may edit when processing has completed
     uploads:
+      ownership_error: "Uploaded files could not be attached because they belong to another user."
       js_templates:
         display_label: Display label
         error: Error

config/locales/hyrax.es.yml

@@ -1750,6 +1750,7 @@ es:
       permissions_message: Actualizando permisos de archivo. Esto puede tardar unos minutos. Quizás quiera actualizar su navegador o regresar a este registro posteriormente para ver los permisos actualizados.
       processing: El archivo se está procesando; se puede editar en cuanto termine el procesamiento
     uploads:
+      ownership_error: "Los archivos subidos no se pudieron adjuntar porque pertenecen a otro usuario."
       js_templates:
         display_label: Mostrar etiqueta
         error: Error

config/locales/hyrax.fr.yml

@@ -1748,6 +1748,7 @@ fr:
       permissions_message: Mise à jour des autorisations de fichier. Cela peut prendre quelques minutes. Vous pouvez actualiser votre navigateur ou revenir à cet enregistrement plus tard pour voir les autorisations de fichier mises à jour.
       processing: Le fichier est en cours de traitement; Vous pouvez modifier lorsque le traitement est terminé
     uploads:
+      ownership_error: "Les fichiers téléversés n'ont pas pu être joints car ils appartiennent à un autre utilisateur."
       js_templates:
         display_label: Étiquette d'affichage
         error: Erreur

config/locales/hyrax.it.yml

@@ -1747,6 +1747,7 @@ it:
       permissions_message: Aggiornamento delle autorizzazioni di file. Questo potrebbe richiedere alcuni minuti. Potresti voler aggiornare il tuo browser o tornare a questo disco in seguito per vedere le autorizzazioni di file aggiornate.
       processing: Il file è in fase di elaborazione; Puoi modificare quando l'elaborazione è terminata
     uploads:
+      ownership_error: "I file caricati non possono essere allegati perché appartengono a un altro utente."
       js_templates:
         display_label: Etichetta di visualizzazione
         error: Errore

config/locales/hyrax.pt-BR.yml

@@ -1740,6 +1740,7 @@ pt-BR:
       permissions_message: Atualizando permissões de arquivo. Isto pode levar alguns minutos. Você pode recarregar seu navegador ou retornar a este registro mais tarde para ver as permissões de arquivo atualizadas.
       processing: O arquivo está sendo processado; você pode editar quando o processamento for concluído.
     uploads:
+      ownership_error: "Os arquivos enviados não puderam ser anexados porque pertencem a outro usuário."
       js_templates:
         display_label: Etiqueta de exibição
         error: Erro

config/locales/hyrax.zh.yml

@@ -1745,6 +1745,7 @@ zh:
       permissions_message: 文件许可正在更新中。稍等片刻。您可能需要刷新浏览器或稍后回访查询该文件访问许可是否更新
       processing: 文件正在处理。 等处理完毕，您可以开始编辑。
     uploads:
+      ownership_error: "无法附加上传的文件，因为它们属于其他用户。"
       js_templates:
         display_label: 显示标签
         error: 错误

spec/controllers/hyrax/dashboard/collections_controller_spec.rb

@@ -503,7 +503,7 @@
         end
 
         context "updating a collections branding metadata" do
-          let(:uploaded) { FactoryBot.create(:uploaded_file) }
+          let(:uploaded) { FactoryBot.create(:uploaded_file, user: user) }
 
           it "saves banner metadata" do
             put :update, params: { id: collection,
@@ -532,8 +532,21 @@
               .to exist
           end
 
+          it "refuses branding files owned by another user" do
+            foreign = FactoryBot.create(:uploaded_file)
+
+            put :update, params: { id: collection,
+                                   banner_files: [foreign.id],
+                                   collection: { creator: ['Emily'] } }
+
+            expect(response).to be_redirect
+            expect(flash[:alert]).to eq I18n.t('hyrax.uploads.ownership_error')
+            expect(CollectionBrandingInfo.where(collection_id: collection.id.to_s, role: "banner"))
+              .not_to exist
+          end
+
           context 'where the linkurl is not a valid http|http link' do
-            let(:uploaded) { FactoryBot.create(:uploaded_file) }
+            let(:uploaded) { FactoryBot.create(:uploaded_file, user: user) }
 
             it "does not save linkurl containing html; target_url is empty" do
               put :update, params: { id: collection,

spec/controllers/hyrax/file_sets_controller_spec.rb

@@ -663,6 +663,44 @@
           end
         end
 
+        context "when the staged file is submitted under the canonical uploaded_files parameter" do
+          it "ingests it like the legacy files_files parameter" do
+            new_file = FactoryBot.create(:uploaded_file, user: user, file: File.open("spec/fixtures/4-20.png"))
+            expect(ValkyrieIngestJob).to receive(:perform_later).with(new_file)
+
+            post :update, params: { id: file_set, uploaded_files: [new_file.id.to_s] }
+
+            expect(new_file.reload.file_set_uri).to eq file_set.id.to_s
+          end
+        end
+
+        context "when the staged file is submitted under the legacy files_files parameter" do
+          it "warns that the parameter is deprecated" do
+            allow(Deprecation).to receive(:warn)
+            new_file = FactoryBot.create(:uploaded_file, user: user, file: File.open("spec/fixtures/4-20.png"))
+
+            post :update, params: { id: file_set, files_files: [new_file.id.to_s] }
+
+            expect(Deprecation).to have_received(:warn).with(a_string_including('files_files'))
+          end
+        end
+
+        context "when the staged file belongs to another user" do
+          let(:other_user) { create(:user) }
+
+          it "refuses to ingest it" do
+            foreign = FactoryBot.create(:uploaded_file, user: other_user, file: File.open("spec/fixtures/4-20.png"))
+            expect(ValkyrieIngestJob).not_to receive(:perform_later)
+
+            post :update, params: { id: file_set, files_files: [foreign.id.to_s] }
+
+            # this context stubs redirect_to (see the containing before block),
+            # so assert the ownership handler issued the redirect through it
+            expect(controller).to have_received(:redirect_to)
+              .with(anything, hash_including(alert: I18n.t('hyrax.uploads.ownership_error')))
+          end
+        end
+
         context "with two existing versions from different users", :perform_enqueued do
           let(:second_user) { create(:user) }
           let(:second_file) { FactoryBot.create(:uploaded_file, user: second_user, file: File.open('spec/fixtures/4-20.png'), file_set_uri: file_set.id.to_s) }

spec/controllers/hyrax/monographs_controller_spec.rb

@@ -9,6 +9,22 @@
   before { sign_in user }
 
   describe "#update" do
+    context "with uploaded files owned by another user" do
+      let(:work) { FactoryBot.valkyrie_create(:comet_in_moominland, edit_users: [user]) }
+      let(:other_user) { FactoryBot.create(:user) }
+      let(:foreign_upload) { FactoryBot.create(:uploaded_file, user: other_user) }
+
+      it "refuses to attach them" do
+        expect(Hyrax::WorkUploadsHandler).not_to receive(:new)
+
+        patch :update, params: { id: work.id,
+                                 monograph: { title: ['comet in moominland'] },
+                                 uploaded_files: [foreign_upload.id.to_s] }
+
+        expect(response).to be_redirect
+        expect(flash[:alert]).to eq I18n.t('hyrax.uploads.ownership_error')
+      end
+    end
     context "when updating work members" do
       let(:work) { FactoryBot.valkyrie_create(:comet_in_moominland, :with_member_works) }
       let(:child1) { Hyrax.query_service.find_by(id: work.member_ids.first) }