From d0de6c9380d2801537b441f27d419277d8794066 Mon Sep 17 00:00:00 2001 From: Jane Sandberg Date: Wed, 11 Dec 2024 08:34:56 -0800 Subject: [PATCH] Move excessive paging out of search builder into controllers It doesn't have anything to do with building a search to send to solr, it is more about validating the HTTP requests from the user --- app/controllers/catalog_controller.rb | 2 + .../concerns/orangelight/excessive_paging.rb | 23 ++++++++ app/models/search_builder.rb | 22 +------ spec/controllers/bookmarks_controller_spec.rb | 4 ++ spec/controllers/catalog_controller_spec.rb | 16 +++++ spec/models/search_builder_spec.rb | 58 ------------------- 6 files changed, 46 insertions(+), 79 deletions(-) create mode 100644 app/controllers/concerns/orangelight/excessive_paging.rb diff --git a/app/controllers/catalog_controller.rb b/app/controllers/catalog_controller.rb index a4814e042..6c1fcf2f8 100644 --- a/app/controllers/catalog_controller.rb +++ b/app/controllers/catalog_controller.rb @@ -7,9 +7,11 @@ class CatalogController < ApplicationController include BlacklightRangeLimit::ControllerOverride include Orangelight::Catalog include Orangelight::Stackmap + include Orangelight::ExcessivePaging include BlacklightHelper before_action :redirect_browse + before_action :deny_excessive_paging rescue_from Blacklight::Exceptions::RecordNotFound do alma_id = "99#{params[:id]}3506421" diff --git a/app/controllers/concerns/orangelight/excessive_paging.rb b/app/controllers/concerns/orangelight/excessive_paging.rb new file mode 100644 index 000000000..fb9bbd7d1 --- /dev/null +++ b/app/controllers/concerns/orangelight/excessive_paging.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true +module Orangelight + module ExcessivePaging + private + + def deny_excessive_paging + page = params[:page].to_i || 0 + return if page <= 1 + return if (has_search_parameters? || advanced_search? || bookmarks_page?) && page < 1000 + render plain: "excessive paging", status: :bad_request + end + + def advanced_search? + %w[advanced numismatics].include?(params[:advanced_type]) || + params[:search_field] == 'advanced' || + %w[advanced numismatics].include?(action_name) + end + + def bookmarks_page? + instance_of?(BookmarksController) + end + end +end diff --git a/app/models/search_builder.rb b/app/models/search_builder.rb index 8e0163183..c9489f0c7 100644 --- a/app/models/search_builder.rb +++ b/app/models/search_builder.rb @@ -8,7 +8,7 @@ class SearchBuilder < Blacklight::SearchBuilder default_processor_chain.unshift(:conditionally_configure_json_query_dsl) self.default_processor_chain += %i[parslet_trick cleanup_boolean_operators - cjk_mm wildcard_char_strip excessive_paging_error + cjk_mm wildcard_char_strip only_home_facets prepare_left_anchor_search series_title_results pul_holdings html_facets numismatics_facets numismatics_advanced @@ -73,22 +73,6 @@ def only_home_facets(solr_parameters) solr_parameters['facet.pivot'] = [] end - # Determines whether or not the user is requesting an excessively high page of results - # @param [ActionController::Parameters] params - # @return [Boolean] - def excessive_paging_error(_solr_parameters) - raise ActionController::BadRequest, "excessive paging" if excessive_paging? - end - - # Determines whether or not the user is requesting an excessively high page of results - # @return [Boolean] - def excessive_paging? - page = blacklight_params[:page].to_i || 0 - return false if page <= 1 - return false if (search_parameters? || advanced_search? || bookmarks_page?) && page < 1000 - true - end - ## # Check if we are on an advanced search page # @return [Boolean] @@ -153,8 +137,4 @@ def add_edismax(advanced_fields:) blacklight_config.search_fields[field]['clause_params'] = { edismax: } end end - - def bookmarks_page? - blacklight_params[:controller] == 'bookmarks' - end end diff --git a/spec/controllers/bookmarks_controller_spec.rb b/spec/controllers/bookmarks_controller_spec.rb index dceb1a96e..d2ba25226 100644 --- a/spec/controllers/bookmarks_controller_spec.rb +++ b/spec/controllers/bookmarks_controller_spec.rb @@ -48,5 +48,9 @@ get :index, params: { page: 2 } expect(response).to have_http_status(:ok) end + it 'errors when paging is excessive' do + get :index, params: { page: 1500 } + expect(response).to have_http_status(:bad_request) + end end end diff --git a/spec/controllers/catalog_controller_spec.rb b/spec/controllers/catalog_controller_spec.rb index 442b1b7e2..0b937739c 100644 --- a/spec/controllers/catalog_controller_spec.rb +++ b/spec/controllers/catalog_controller_spec.rb @@ -82,10 +82,26 @@ get :index, params: { q: 'asdf', page: 2 } expect(response.status).to eq(200) end + it 'does not error when paging is reasonable and the search is a facet' do + get :index, params: { f: { format: 'anything' }, page: 2 } + expect(response.status).to eq(200) + end + it 'does not error when query is empty and paging is reasonable' do + get :index, params: { q: '', search_field: 'all_fields', page: 2 } + expect(response.status).to eq(200) + end + it 'does not error when paging is reasonable and query is from advanced search' do + get :index, params: { clause: { '0': { query: 'dog' } }, page: 2 } + expect(response.status).to eq(200) + end it 'errors when paging is excessive' do get :index, params: { q: 'asdf', page: 1500 } expect(response.status).to eq(400) end + it 'errors when paging is reasonable but there is no query or facet' do + get :index, params: { page: 3 } + expect(response.status).to eq(400) + end end describe 'home page' do diff --git a/spec/models/search_builder_spec.rb b/spec/models/search_builder_spec.rb index f45bda1b0..16409fc83 100644 --- a/spec/models/search_builder_spec.rb +++ b/spec/models/search_builder_spec.rb @@ -9,64 +9,6 @@ let(:scope) { Blacklight::SearchService.new config: blacklight_config, search_state: state } let(:state) { Blacklight::SearchState.new({}, blacklight_config) } - describe '#excessive_paging' do - let(:excessive) { 9999 } - let(:reasonable) { 123 } - - it 'allows reasonable paging with a search query' do - search_builder.blacklight_params[:page] = reasonable - search_builder.blacklight_params[:q] = 'anything' - expect(search_builder.excessive_paging?).to be false - end - - it 'allows reasonable paging with a facet query' do - search_builder.blacklight_params[:page] = reasonable - search_builder.blacklight_params[:f] = 'anything' - expect(search_builder.excessive_paging?).to be false - end - - it 'returns false if the search is empty' do - search_builder.blacklight_params[:page] = reasonable - search_builder.blacklight_params[:q] = '' - expect(search_builder.excessive_paging?).to be false - end - - it 'does not allow paging without a search or facet' do - search_builder.blacklight_params[:page] = reasonable - expect(search_builder.excessive_paging?).to be true - end - - it 'does not allow excessive paging with a search query' do - search_builder.blacklight_params[:page] = excessive - search_builder.blacklight_params[:q] = 'anything' - expect(search_builder.excessive_paging?).to be true - end - - it 'does not allow excessive paging with a facet query' do - search_builder.blacklight_params[:page] = excessive - search_builder.blacklight_params[:f] = 'anything' - expect(search_builder.excessive_paging?).to be true - end - - it 'allows paging for advanced search' do - search_builder.blacklight_params[:page] = reasonable - search_builder.blacklight_params[:search_field] = 'advanced' - expect(search_builder.excessive_paging?).to be false - end - - it 'allows paging for bookmarks controller' do - search_builder.blacklight_params[:page] = reasonable - search_builder.blacklight_params[:controller] = 'bookmarks' - expect(search_builder.excessive_paging?).to be false - end - - it 'handles query ending with empty parenthesis' do - search_builder.blacklight_params[:q] = 'hello world ()' - search_builder.parslet_trick({}) - expect(search_builder.blacklight_params[:q].end_with?("()")).to be false - end - end - describe '#cleanup_boolean_operators' do let(:solr_parameters) do { q: 'Douglas fir' }