diff --git a/Koha/Exceptions/Report.pm b/Koha/Exceptions/Report.pm new file mode 100644 index 0000000000..141003d31e --- /dev/null +++ b/Koha/Exceptions/Report.pm @@ -0,0 +1,14 @@ +package Koha::Exceptions::Report; + +use Modern::Perl; + +use Exception::Class ( + 'Koha::Exceptions::Report' => { description => "Something went wrong!" }, + 'Koha::Exceptions::Report::DuplicateRunning' => { + isa => 'Koha::Exceptions::Report', + description => "The configured per-user limit on simultaneous runs of this report has been reached", + fields => [ 'report_id', 'user_id', 'limit' ], + }, +); + +1; diff --git a/Koha/Report.pm b/Koha/Report.pm index 13c8a68f48..57ecfb21af 100644 --- a/Koha/Report.pm +++ b/Koha/Report.pm @@ -18,6 +18,7 @@ package Koha::Report; use Modern::Perl; use Koha::Database; +use Koha::Exceptions::Report; use Koha::Reports; #use Koha::DateUtils qw( dt_from_string output_pref ); @@ -174,6 +175,13 @@ C to detect concurrent runs of the same report. External log/audit tooling that consumes Koha SQL should treat this format as part of the public contract. +If C is set in C +to a non-zero value and the current userenv already has at least that many +runs of the same report in flight, this method throws +L instead of returning. All +report-execution entry points (the staff guided-reports UI, C +and C) go through here so the limit is enforced uniformly. + =cut sub prep_report { @@ -242,6 +250,19 @@ sub prep_report { my $report_id = $self->id; my $user_id = C4::Context->userenv ? C4::Context->userenv->{number} : 0; + + my $duplicate_limit = C4::Context->config('duplicate_running_reports_per_user_limit'); + if ( $duplicate_limit && $user_id ) { + my $running = Koha::Reports->running( { report_id => $report_id, user_id => $user_id } ); + if ( $running->count >= $duplicate_limit ) { + Koha::Exceptions::Report::DuplicateRunning->throw( + report_id => $report_id, + user_id => $user_id, + limit => $duplicate_limit, + ); + } + } + $sql .= " /* { saved_sql.id: $report_id } { user_id: $user_id } */"; return $sql, $headers; diff --git a/opac/svc/report b/opac/svc/report index 3c2812f3ce..2eba1b1567 100755 --- a/opac/svc/report +++ b/opac/svc/report @@ -63,7 +63,19 @@ if ($cache_active) { unless ($json_text) { my $limit = C4::Context->preference("SvcMaxReportRows") || 10; - my ( $sql, undef ) = $report_rec->prep_report( \@param_names, \@sql_params ); + my ( $sql, undef ); + eval { ( $sql, undef ) = $report_rec->prep_report( \@param_names, \@sql_params ); }; + if ( my $e = $@ ) { + if ( ref($e) eq 'Koha::Exceptions::Report::DuplicateRunning' ) { + print $query->header( + -type => 'application/json', + -status => '429 Too Many Requests', + ); + print encode_json( { error => 'duplicate_running_report' } ); + exit 0; + } + die $e; + } my ( $sth, $errors ) = execute_query( { diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index 2d2346d054..bf4a335a93 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -824,23 +824,6 @@ if ( $op eq 'run' ) { my $template_id = $input->param('template'); my $want_full_chart = $input->param('want_full_chart') || 0; - my $duplicate_running_reports_per_user_limit = C4::Context->config('duplicate_running_reports_per_user_limit'); - my $duplicate_running_reports; - if ( $duplicate_running_reports_per_user_limit && C4::Context->userenv ) { - - # This is a TOCTOU check: between the count below and the actual - # query execution further down, a parallel request can slip past the - # limit. Worst case is one extra concurrent run, which is acceptable - # for the intended use case (preventing accidental double-clicks / - # multi-tab submissions) and avoids the cost of a serializing lock. - $duplicate_running_reports = Koha::Reports->running( - { - report_id => $report_id, - user_id => C4::Context->userenv->{number}, - } - ); - } - # offset algorithm if ( $input->param('page') ) { $offset = ( $input->param('page') - 1 ) * $limit; @@ -1026,22 +1009,27 @@ if ( $op eq 'run' ) { 'id' => $report_id, 'template_id' => $template_id, ); - } elsif ( $duplicate_running_reports - && $duplicate_running_reports->count >= $duplicate_running_reports_per_user_limit ) - { - $template->param( - 'sql' => $sql, - 'original_sql' => $original_sql, - 'id' => $report_id, - 'execute' => 1, - 'name' => $name, - 'notes' => $notes, - 'errors' => [ { duplicate_running_report => 1 } ], - 'sql_params' => \@sql_params, - 'param_names' => \@param_names, - ); } else { - my ( $sql, $header_types ) = $report->prep_report( \@param_names, \@sql_params ); + my ( $sql, $header_types ); + eval { ( $sql, $header_types ) = $report->prep_report( \@param_names, \@sql_params ); }; + if ( my $e = $@ ) { + if ( ref($e) eq 'Koha::Exceptions::Report::DuplicateRunning' ) { + $template->param( + 'sql' => $original_sql, + 'original_sql' => $original_sql, + 'id' => $report_id, + 'execute' => 1, + 'name' => $name, + 'notes' => $notes, + 'errors' => [ { duplicate_running_report => 1 } ], + 'sql_params' => \@sql_params, + 'param_names' => \@param_names, + ); + output_html_with_http_headers $input, $cookie, $template->output; + exit; + } + die $e; + } $template->param( header_types => $header_types ); my ( $sth, $errors ) = execute_query( { diff --git a/svc/report b/svc/report index 905dff8fd9..838f948f0a 100755 --- a/svc/report +++ b/svc/report @@ -65,7 +65,19 @@ if ($cache_active) { unless ($json_text) { my $limit = C4::Context->preference("SvcMaxReportRows") || 10; - my ( $sql, undef ) = $report_rec->prep_report( \@param_names, \@sql_params ); + my ( $sql, undef ); + eval { ( $sql, undef ) = $report_rec->prep_report( \@param_names, \@sql_params ); }; + if ( my $e = $@ ) { + if ( ref($e) eq 'Koha::Exceptions::Report::DuplicateRunning' ) { + print $query->header( + -type => 'application/json', + -status => '429 Too Many Requests', + ); + print encode_json( { error => 'duplicate_running_report' } ); + exit 0; + } + die $e; + } my ( $sth, $errors ) = execute_query( { diff --git a/t/db_dependent/Koha/Reports.t b/t/db_dependent/Koha/Reports.t index 462d2232d3..2cb595c8bc 100755 --- a/t/db_dependent/Koha/Reports.t +++ b/t/db_dependent/Koha/Reports.t @@ -17,9 +17,10 @@ use Modern::Perl; +use Test::Exception; use Test::MockModule; use Test::NoWarnings; -use Test::More tests => 11; +use Test::More tests => 12; use Koha::Report; use Koha::Reports; @@ -102,6 +103,43 @@ subtest 'prep_report' => sub { is_deeply( $headers, { 'itemnumber for batch' => 'itemnumber' } ); }; +subtest 'prep_report throws when duplicate-running limit is exceeded' => sub { + plan tests => 4; + + my $report = Koha::Report->new( { report_name => 'duplicate_running_test', savedsql => 'SELECT 1' } )->store; + + # Pretend a real user is logged in so prep_report's user_id check fires. + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + t::lib::Mocks::mock_userenv( { patron => $patron } ); + + # Pretend two copies of this report are already in flight. + my $reports_mock = Test::MockModule->new('Koha::Reports'); + $reports_mock->mock( + 'running', + sub { + my ( $class, $params ) = @_; + return $class->search( { id => $report->id } ) if $params->{report_id}; + return $class->search( { id => undef } ); + } + ); + + t::lib::Mocks::mock_config( 'duplicate_running_reports_per_user_limit', 0 ); + lives_ok { $report->prep_report( [], [] ) } 'limit=0 disables the guard'; + + t::lib::Mocks::mock_config( 'duplicate_running_reports_per_user_limit', 5 ); + lives_ok { $report->prep_report( [], [] ) } 'limit not yet reached, prep_report returns normally'; + + t::lib::Mocks::mock_config( 'duplicate_running_reports_per_user_limit', 1 ); + my $exception; + eval { $report->prep_report( [], [] ); }; + $exception = $@; + isa_ok( + $exception, 'Koha::Exceptions::Report::DuplicateRunning', + 'limit reached -> DuplicateRunning exception' + ); + is( $exception->limit, 1, 'exception carries the configured limit' ); +}; + subtest 'is_sql_valid' => sub { plan tests => 3 + 6 * 2; my @badwords = ( 'UPDATE', 'DELETE', 'DROP', 'INSERT', 'SHOW', 'CREATE' );