Bug 41918: (QA follow-up) Enforce duplicate-running limit in the model

The previous patches enforced duplicate_running_reports_per_user_limit
only in reports/guided_reports.pl. Two other code paths run saved SQL
reports for an authenticated user and bypassed the guard entirely:

- svc/report (staff JSON endpoint, eg. used for chart widgets and any
  caller hitting /cgi-bin/koha/svc/report?id=N directly)
- opac/svc/report (OPAC JSON endpoint)

A user who hit their staff-UI limit could still spawn additional copies
of the same long-running report via either svc endpoint, defeating the
guard.

Push the check into Koha::Report::prep_report itself, which is the one
function all three callers go through. When the limit is reached
prep_report now throws a new Koha::Exceptions::Report::DuplicateRunning
(carrying report_id, user_id and the configured limit). The guided
reports CGI catches it and re-renders the existing "report is already
running" alert; both svc/report endpoints catch it and return HTTP 429
Too Many Requests with a JSON { error: "duplicate_running_report" }
payload.

Notes:
- The pre-flight Koha::Reports->running call is removed from the
  guided-reports CGI; the model now owns that policy and the
  controller is just rendering. The TOCTOU window remains identical
  in size and is the same intentional trade-off as before.
- Limit=0 (the default) and absent userenv both bypass the check, so
  CLI cronjobs (misc/cronjobs/runreport.pl) and direct DB users are
  unaffected.

Test plan:
1) prove t/db_dependent/Koha/Reports.t - now includes a new subtest
   covering limit=0 (lives), under limit (lives) and at limit (throws
   Koha::Exceptions::Report::DuplicateRunning).
2) Set duplicate_running_reports_per_user_limit=1 in koha-conf.xml,
   restart_all.
3) Open a long-running report in two staff tabs - second tab shows
   the "report is already running" alert (unchanged behaviour).
4) While one run is in flight, hit /cgi-bin/koha/svc/report?id=N as
   the same user - now returns HTTP 429 with the JSON error payload
   (previously: ran a second copy of the query).
5) Same check on /cgi-bin/koha/opac/svc/report?id=N.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

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;

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<information_schema.processlist> 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<duplicate_running_reports_per_user_limit> is set in C<koha-conf.xml>
+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<Koha::Exceptions::Report::DuplicateRunning> instead of returning. All
+report-execution entry points (the staff guided-reports UI, C<svc/report>
+and C<opac/svc/report>) 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;

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(
         {

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(
                 {

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(
         {

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' );