Add Bulkrax::Ability concern and enforce importer/exporter ownership

app/controllers/bulkrax/application_controller.rb

@@ -4,5 +4,19 @@ module Bulkrax
   class ApplicationController < ::ApplicationController
     helper Rails.application.class.helpers
     protect_from_forgery with: :exception
+
+    # Rescue CanCan::AccessDenied in all Bulkrax controllers.  HTML requests are
+    # redirected to the host-app root with an alert; JSON requests receive a 403
+    # response.  Defining the handler here (rather than in individual controllers)
+    # keeps error handling in one place and consistent across all resources.
+    rescue_from CanCan::AccessDenied do |exception|
+      respond_to do |format|
+        format.html do
+          flash[:alert] = exception.message
+          redirect_to main_app.root_path
+        end
+        format.json { render json: { error: exception.message }, status: :forbidden }
+      end
+    end
   end
 end

app/controllers/bulkrax/entries_controller.rb

@@ -4,7 +4,6 @@ module Bulkrax
   class EntriesController < ApplicationController
     include Hyrax::ThemedLayoutController if defined?(::Hyrax)
     before_action :authenticate_user!
-    before_action :check_permissions
     with_themed_layout 'dashboard' if defined?(::Hyrax)
 
     def show
@@ -17,6 +16,8 @@ def show
 
     def update
       @entry = Entry.find(params[:id])
+      authorize! :update, @entry
+
       type = case @entry.type.downcase
              when /fileset/
                'file_set'
@@ -25,8 +26,8 @@ def update
              else
                'work'
              end
-      item = @entry.importerexporter
       # do not run counters as it loads the whole parser
+      item = @entry.importerexporter
       current_run = item.current_run(skip_counts: true)
       @entry.set_status_info('Pending', current_run)
       ScheduleRelationshipsJob.set(wait: 5.minutes).perform_later(importer_id: @entry.importer.id)
@@ -44,6 +45,9 @@ def update
 
     def destroy
       @entry = Entry.find(params[:id])
+      authorize! :destroy, @entry
+
+      item = @entry.importerexporter
       @status = ""
       begin
         work = @entry.factory&.find
@@ -59,7 +63,6 @@ def destroy
         @status = "Error: #{e.message}"
       end
 
-      item = @entry.importerexporter
       entry_path = item.class.to_s.include?('Importer') ? bulkrax.importer_entry_path(item.id, @entry.id) : bulkrax.exporter_entry_path(item.id, @entry.id)
 
       redirect_back fallback_location: entry_path, notice: @status
@@ -70,7 +73,9 @@ def destroy
     # GET /importers/1/entries/1
     def show_importer
       @importer = Importer.find(params[:importer_id])
-      @entry = Entry.find(params[:id])
+      authorize! :read, @importer
+      @entry = @importer.entries.find(params[:id])
+      authorize! :read, @entry
 
       return unless defined?(::Hyrax)
       add_breadcrumb t(:'hyrax.controls.home'), main_app.root_path
@@ -83,7 +88,9 @@ def show_importer
     # GET /exporters/1/entries/1
     def show_exporter
       @exporter = Exporter.find(params[:exporter_id])
-      @entry = Entry.find(params[:id])
+      authorize! :read, @exporter
+      @entry = @exporter.entries.find(params[:id])
+      authorize! :read, @entry
 
       return unless defined?(::Hyrax)
       add_breadcrumb t(:'hyrax.controls.home'), main_app.root_path
@@ -92,9 +99,5 @@ def show_exporter
       add_breadcrumb @exporter.name, bulkrax.exporter_path(@exporter.id)
       add_breadcrumb @entry.id
     end
-
-    def check_permissions
-      raise CanCan::AccessDenied unless current_ability.can_import_works? || current_ability.can_export_works?
-    end
   end
 end

app/controllers/bulkrax/exporters_controller.rb

@@ -6,20 +6,25 @@ class ExportersController < ApplicationController
     include Bulkrax::DownloadBehavior
     include Bulkrax::DatatablesBehavior
     before_action :authenticate_user!
-    before_action :check_permissions
-    before_action :set_exporter, only: [:show, :entry_table, :edit, :update, :destroy]
+    # load_and_authorize_resource handles both record loading and per-resource
+    # authorization for all member actions.  Index and exporter_table are
+    # excluded because they operate on a collection (scoped via accessible_by).
+    # Download is excluded because it uses :exporter_id rather than :id.
+    load_and_authorize_resource class: 'Bulkrax::Exporter',
+                                instance_name: :exporter,
+                                except: [:index, :exporter_table, :download]
     with_themed_layout 'dashboard' if defined?(::Hyrax)
 
     # GET /exporters
     def index
       # NOTE: We're paginating this in the browser.
-      @exporters = Exporter.order(created_at: :desc).all
+      @exporters = Exporter.accessible_by(current_ability).order(created_at: :desc)
 
       add_exporter_breadcrumbs if defined?(::Hyrax)
     end
 
     def exporter_table
-      @exporters = Exporter.order(table_order).page(table_page).per(table_per_page)
+      @exporters = Exporter.accessible_by(current_ability).order(table_order).page(table_page).per(table_per_page)
       @exporters = @exporters.where(exporter_table_search) if exporter_table_search.present?
       respond_to do |format|
         format.json { render json: format_exporters(@exporters) }
@@ -45,7 +50,6 @@ def entry_table
 
     # GET /exporters/new
     def new
-      @exporter = Exporter.new
       return unless defined?(::Hyrax)
       add_exporter_breadcrumbs
       add_breadcrumb 'New'
@@ -65,7 +69,6 @@ def edit
 
     # POST /exporters
     def create
-      @exporter = Exporter.new(exporter_params)
       field_mapping_params
 
       if @exporter.save
@@ -107,16 +110,12 @@ def destroy
     # GET /exporters/1/download
     def download
       @exporter = Exporter.find(params[:exporter_id])
+      authorize! :read, @exporter
       send_content
     end
 
     private
 
-    # Use callbacks to share common setup or constraints between actions.
-    def set_exporter
-      @exporter = Exporter.find(params[:id] || params[:exporter_id])
-    end
-
     # Only allow a trusted parameters through.
     def exporter_params
       params[:exporter][:export_source] = params[:exporter]["export_source_#{params[:exporter][:export_from]}".to_sym]
@@ -149,9 +148,5 @@ def add_exporter_breadcrumbs
     def file_path
       "#{@exporter.exporter_export_zip_path}/#{params['exporter']['exporter_export_zip_files']}"
     end
-
-    def check_permissions
-      raise CanCan::AccessDenied unless current_ability.can_export_works?
-    end
   end
 end

app/controllers/bulkrax/guided_imports_controller.rb

@@ -8,7 +8,7 @@ class GuidedImportsController < ::Bulkrax::ApplicationController
     helper Bulkrax::ImportersHelper
 
     before_action :authenticate_user!
-    before_action :check_permissions
+    before_action { authorize! :create, Bulkrax::Importer }
     with_themed_layout 'dashboard' if defined?(::Hyrax)
 
     # trigger form to allow upload
@@ -167,9 +167,5 @@ def add_importer_breadcrumbs
       add_breadcrumb t(:'hyrax.dashboard.breadcrumbs.admin'), hyrax.dashboard_path
       add_breadcrumb 'Importers', bulkrax.importers_path
     end
-
-    def check_permissions
-      raise CanCan::AccessDenied unless current_ability.can_import_works?
-    end
   end
 end

app/controllers/bulkrax/importers_controller.rb

@@ -13,14 +13,33 @@ class ImportersController < ::Bulkrax::ApplicationController
     protect_from_forgery unless: -> { api_request? }
     before_action :token_authenticate!, if: -> { api_request? }, only: [:create, :update, :delete]
     before_action :authenticate_user!, unless: -> { api_request? }
-    before_action :check_permissions
-    before_action :set_importer, only: [:show, :entry_table, :edit, :update, :destroy, :original_file]
+    # load_and_authorize_resource covers standard CRUD member actions for
+    # non-API requests.  Actions that use :importer_id rather than :id, or
+    # that are collection-scoped, are excluded and handled by separate
+    # before_actions below.
+    load_and_authorize_resource class: 'Bulkrax::Importer',
+                                instance_name: :importer,
+                                except: [:index, :importer_table, :sample_csv_file, :external_sets,
+                                         :continue, :upload_corrected_entries, :upload_corrected_entries_file,
+                                         :export_errors, :original_file],
+                                unless: -> { api_request? }
+    # For API requests, fall back to simple find so existing consumers are
+    # unaffected while the token-to-user wiring is not yet implemented.
+    before_action :set_importer_for_api,
+                  only: [:show, :entry_table, :edit, :update, :destroy, :original_file],
+                  if: -> { api_request? }
+    # Actions that reference importers by :importer_id (not :id)
+    before_action :load_and_authorize_importer_by_importer_id,
+                  only: [:continue, :upload_corrected_entries, :upload_corrected_entries_file, :export_errors, :original_file]
     with_themed_layout 'dashboard' if defined?(::Hyrax)
 
     # GET /importers
     def index
       # NOTE: We're paginating this in the browser.
       if api_request?
+        # TODO: Scope API index by token owner once token-to-user wiring is in
+        # place.  Tracked in [ISSUE].  Until then, API clients see all importers
+        # to preserve backward-compatibility with existing API consumers.
         @importers = Importer.order(created_at: :desc).all
         json_response('index')
       elsif defined?(::Hyrax)
@@ -30,7 +49,13 @@ def index
 
     def importer_table
       order = table_order.presence || Arel.sql('last_imported_at DESC NULLS LAST')
-      @importers = Importer.order(order).page(table_page).per(table_per_page)
+      # TODO: API requests bypass ownership scoping here too; see index TODO.
+      @importers = if api_request?
+                     Importer.all
+                   else
+                     Importer.accessible_by(current_ability)
+                   end
+      @importers = @importers.order(order).page(table_page).per(table_per_page)
       @importers = @importers.where(importer_table_search) if importer_table_search.present?
       respond_to do |format|
         format.json { render json: format_importers(@importers) }
@@ -58,7 +83,6 @@ def entry_table
 
     # GET /importers/new
     def new
-      @importer = Importer.new
       if api_request?
         json_response('new')
       elsif defined?(::Hyrax)
@@ -95,12 +119,13 @@ def create
       # rubocop:disable Style/IfInsideElse
       if api_request?
         return return_json_response unless valid_create_params?
+        # load_and_authorize_resource is skipped for API; build the record here.
+        @importer ||= Importer.new(importer_params)
       end
       uploads = uploaded_files_scope
       file = file_param
       cloud_files = cloud_params
 
-      @importer = Importer.new(importer_params)
       field_mapping_params
       @importer.validate_only = true if params[:commit] == 'Create and Validate'
       # the following line is needed to handle updating remote files of a FileSet
@@ -177,15 +202,13 @@ def destroy
 
     # PUT /importers/1
     def continue
-      @importer = Importer.find(params[:importer_id])
       params[:importer] = { name: @importer.name }
       @importer.validate_only = false
       update
     end
 
     # GET /importer/1/upload_corrected_entries
     def upload_corrected_entries
-      @importer = Importer.find(params[:importer_id])
       return unless defined?(::Hyrax)
       add_breadcrumb t(:'hyrax.controls.home'), main_app.root_path
       add_breadcrumb t(:'hyrax.dashboard.breadcrumbs.admin'), hyrax.dashboard_path
@@ -197,7 +220,6 @@ def upload_corrected_entries
     # POST /importer/1/upload_corrected_entries_file
     def upload_corrected_entries_file
       file = params[:importer][:parser_fields].delete(:file)
-      @importer = Importer.find(params[:importer_id])
       if file.present?
         @importer[:parser_fields]['partial_import_file_path'] = @importer.parser.write_partial_import_file(file)
         @importer.save
@@ -242,18 +264,31 @@ def original_file
 
     # GET /importers/1/export_errors
     def export_errors
-      @importer = Importer.find(params[:importer_id])
       @importer.write_errored_entries_file
       send_content
     end
 
     private
 
-    # Use callbacks to share common setup or constraints between actions.
-    def set_importer
+    # Load @importer for API requests (no CanCan authorization — the API path
+    # does not yet resolve a token to a current_user, so ownership rules cannot
+    # be evaluated).
+    # TODO: Remove once token-to-user wiring is complete and CanCan rules apply
+    # uniformly to API requests.  Tracked in [ISSUE].
+    def set_importer_for_api
       @importer = Importer.find(params[:id] || params[:importer_id])
     end
 
+    # Load and authorize @importer for actions that identify the importer by
+    # :importer_id rather than :id (e.g. continue, upload_corrected_entries).
+    # Uses the action name directly so that alias_action mappings defined in
+    # bulkrax_default_abilities apply (e.g. :export_errors → :read,
+    # :continue → :update).
+    def load_and_authorize_importer_by_importer_id
+      @importer = Importer.find(params[:importer_id])
+      authorize! action_name.to_sym, @importer
+    end
+
     def importable_params
       params.except(:selected_files)
     end
@@ -365,10 +400,6 @@ def set_files_parser_fields
       end
       @importer.save
     end
-
-    def check_permissions
-      raise CanCan::AccessDenied unless current_ability.can_import_works?
-    end
   end
   # rubocop:enable Metrics/ClassLength
 end

app/controllers/concerns/bulkrax/datatables_behavior.rb

@@ -102,8 +102,8 @@ def format_importers(importers)
       end
       {
         data: result,
-        recordsTotal: Bulkrax::Importer.count,
-        recordsFiltered: Bulkrax::Importer.count
+        recordsTotal: Bulkrax::Importer.accessible_by(current_ability).count,
+        recordsFiltered: Bulkrax::Importer.accessible_by(current_ability).count
       }
     end
 
@@ -119,8 +119,8 @@ def format_exporters(exporters)
       end
       {
         data: result,
-        recordsTotal: Bulkrax::Exporter.count,
-        recordsFiltered: Bulkrax::Exporter.count
+        recordsTotal: Bulkrax::Exporter.accessible_by(current_ability).count,
+        recordsFiltered: Bulkrax::Exporter.accessible_by(current_ability).count
       }
     end
 
@@ -146,8 +146,10 @@ def format_entries(entries, item)
     def entry_util_links(e, item)
       links = []
       links << view_context.link_to(view_context.raw('<span class="fa fa-info-circle"></span>'), view_context.item_entry_path(item, e))
-      links << "<a class='fa fa-repeat' data-toggle='modal' data-target='#bulkraxItemModal' data-entry-id='#{e.id}'></a>" if view_context.an_importer?(item)
-      links << view_context.link_to(view_context.raw('<span class="fa fa-trash"></span>'), view_context.item_entry_path(item, e), method: :delete, data: { confirm: 'This will delete the entry and any work associated with it. Are you sure?' })
+      if can?(:update, item)
+        links << "<a class='fa fa-repeat' data-toggle='modal' data-target='#bulkraxItemModal' data-entry-id='#{e.id}'></a>" if view_context.an_importer?(item)
+        links << view_context.link_to(view_context.raw('<span class="fa fa-trash"></span>'), view_context.item_entry_path(item, e), method: :delete, data: { confirm: 'This will delete the entry and any work associated with it. Are you sure?' })
+      end
       links.join(" ")
     end
 
@@ -166,16 +168,20 @@ def status_message_for(e)
     def importer_util_links(i)
       links = []
       links << view_context.link_to(view_context.raw('<span class="fa fa-info-circle"></span>'), importer_path(i))
-      links << view_context.link_to(view_context.raw('<span class="fa fa-pencil"></span>'), edit_importer_path(i))
-      links << view_context.link_to(view_context.raw('<span class="fa fa-remove"></span>'), i, method: :delete, data: { confirm: 'Are you sure?' })
+      if can?(:update, i)
+        links << view_context.link_to(view_context.raw('<span class="fa fa-pencil"></span>'), edit_importer_path(i))
+        links << view_context.link_to(view_context.raw('<span class="fa fa-remove"></span>'), i, method: :delete, data: { confirm: 'Are you sure?' })
+      end
       links.join(" ")
     end
 
     def exporter_util_links(i)
       links = []
       links << view_context.link_to(view_context.raw('<span class="fa fa-info-circle"></span>'), exporter_path(i))
-      links << view_context.link_to(view_context.raw('<span class="fa fa-pencil"></span>'), edit_exporter_path(i), data: { turbolinks: false })
-      links << view_context.link_to(view_context.raw('<span class="fa fa-remove"></span>'), i, method: :delete, data: { confirm: 'Are you sure?' })
+      if can?(:update, i)
+        links << view_context.link_to(view_context.raw('<span class="fa fa-pencil"></span>'), edit_exporter_path(i), data: { turbolinks: false })
+        links << view_context.link_to(view_context.raw('<span class="fa fa-remove"></span>'), i, method: :delete, data: { confirm: 'Are you sure?' })
+      end
       links.join(" ")
     end