[Bug 41918] New: Prevent users from running the same report multiple times concurrently
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Bug ID: 41918 Summary: Prevent users from running the same report multiple times concurrently Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Reports Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: lisette@bywatersolutions.com We have seen users reload and run long running reports when they do not get results in a timely manner. This can lead to server issues as the database resources are used up running many copies of the same report. We should allow any given report to only be run one execution at a time. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |41919 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41919 [Bug 41919] Limit number of current reports a single user can run simultaneously -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Anneli Österman <anneli.osterman@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |anneli.osterman@koha-suomi. | |fi --- Comment #1 from Anneli Österman <anneli.osterman@koha-suomi.fi> ---
We should allow any given report to only be run one execution at a time.
Do you mean per user or per all users? I think we have reports that several users need to run simultaneously. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Lari Strand <lmstrand@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lmstrand@gmail.com --- Comment #2 from Lari Strand <lmstrand@gmail.com> --- Maybe showing a spinner like in patron search or a textbox with spinner with text like "Running report, please wait..." that disables UI controls that disappears when the user moves away from the page with browser controls or when the report data is fetched would suffice? I tested this concept quickly and it seems decent to me. The browser's own tab spinner is quite unnoticeable and clearer indication that report data is still being fetched might help in this issue. The problem is that the spinner would need to be tied to result page pagination controls and tabs etc. since the whole report gets ran again by changing/pressing them (which could be an issue for another ticket). I also think there's no way we can tie database processes to what is seen in the UI and we definitely need to allow running the same report by different users simultaneously. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #3 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 193918 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193918&action=edit Bug 41918: Add method to see running report queries by user and id Patch from commit 516e7ec -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #4 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 193919 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193919&action=edit Bug 41918: Prevent a given librarian from running the same report multiple times simultaneously Test Plan: 1) create a long running report such as: SELECT COUNT(*) FROM items i1 JOIN items i2 JOIN items i3 JOIN items i4 JOIN items i5 JOIN items i6 2) Run this report multiple times 3) Note the processes are running via SHOW FULL PROCESSLIST from koha-mysql 4) Kill those processes 5) Apply this patch 6) Restart all the things! 7) Try to run the report in two tabs, note the second tab opened says the report is already running! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #5 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 193920 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193920&action=edit Bug 41918: Add ability to disable or change limit via config xml This commit add the option duplicate_running_reports_per_user_limit to koha-conf.xml. If this option is empty or 0, the feature is disabled. If this option is enabled, Koha will limit the number of times a given report can be run of a single Koha user. Test Plan: 1) Apply this patch 2) Set duplicate_running_reports_per_user_limit to 1, verify previous behavior is maintained 2) Set duplicate_running_reports_per_user_limit to 0, verify the feature is disabled 3) Set duplicate_running_reports_per_user_limit to 2, verify Koha limits the number of duplicates of the report to the number specified in duplicate_running_reports_per_user_limit -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #6 from Lari Strand <lmstrand@gmail.com> --- Seems to be working fine when trying to run two instances of the same report from the report view (/cgi-bin/koha/reports/guided_reports.pl?id=x&op=show) when limit is set to 1 in koha-conf.xml. Running a simultaneous report from list view(/cgi-bin/koha/reports/guided_reports.pl?op=list) before/after running one from report view does not block a second run. Should it? I'm not running my Koha with plack so page loads are quite slow but based on my repeated tests this works consistently from the op=show view at least. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Rebecca Coert <rcoert@arlingtonva.us> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |rcoert@arlingtonva.us -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #7 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- (In reply to Lari Strand from comment #6)
Seems to be working fine when trying to run two instances of the same report from the report view (/cgi-bin/koha/reports/guided_reports.pl?id=x&op=show) when limit is set to 1 in koha-conf.xml.
Running a simultaneous report from list view(/cgi-bin/koha/reports/guided_reports.pl?op=list) before/after running one from report view does not block a second run. Should it?
I'm not running my Koha with plack so page loads are quite slow but based on my repeated tests this works consistently from the op=show view at least.
I was unable to recreate that. Does the report show up in the processlist twice? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com --- Comment #8 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- A few testing notes: - SLEEP() can give you a query that takes a while to run without asking much of your machine - On my KTD, duplicate runs of the same report consistently took 10 seconds or so before they displayed the message telling me the run was blocked. So the page would look like the report was starting (spinning wheel, etc) though no process was ever started in the DB - KTD defaults to just two plack workers. You need to increase that in the koha-conf in order to test behavior when duplicate_running_reports_per_user_limit is set to 2 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Alex Carver [Acerock7] <alex@rcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Alex Carver [Acerock7] <alex@rcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193918|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Alex Carver [Acerock7] <alex@rcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193919|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Alex Carver [Acerock7] <alex@rcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193920|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #9 from Alex Carver [Acerock7] <alex@rcls.org> --- Created attachment 197717 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197717&action=edit Bug 41918: Add method to see running report queries by user and id Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #10 from Alex Carver [Acerock7] <alex@rcls.org> --- Created attachment 197718 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197718&action=edit Bug 41918: Prevent a given librarian from running the same report multiple times simultaneously Test Plan: 1) create a long running report such as: SELECT COUNT(*) FROM items i1 JOIN items i2 JOIN items i3 JOIN items i4 JOIN items i5 JOIN items i6 2) Run this report multiple times 3) Note the processes are running via SHOW FULL PROCESSLIST from koha-mysql 4) Kill those processes 5) Apply this patch 6) Restart all the things! 7) Try to run the report in two tabs, note the second tab opened says the report is already running! Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #11 from Alex Carver [Acerock7] <alex@rcls.org> --- Created attachment 197719 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197719&action=edit Bug 41918: Add ability to disable or change limit via config xml This commit add the option duplicate_running_reports_per_user_limit to koha-conf.xml. If this option is empty or 0, the feature is disabled. If this option is enabled, Koha will limit the number of times a given report can be run of a single Koha user. Test Plan: 1) Apply this patch 2) Set duplicate_running_reports_per_user_limit to 1, verify previous behavior is maintained 2) Set duplicate_running_reports_per_user_limit to 0, verify the feature is disabled 3) Set duplicate_running_reports_per_user_limit to 2, verify Koha limits the number of duplicates of the report to the number specified in duplicate_running_reports_per_user_limit Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- Hmm this is interesting. It does rely on your database user not having the PROCESS privilege. If your database user isn't set up correctly, you could get incorrect results on a shared server. But then you'll have other problems too anyway. -- I think it would be a good idea to change "user_id" to "borrowernumber" or "patron_id", because the POD in Koha/Reports.pm says it's $user->userid but it's not. It's using the borrowernumber. Initially, I was concerned about seeing $user->userid, because that would be a SQL injection vulnerability, but the data injected into the SQL in koha/Report.pm looks like it should be system controlled. (Still might not hurt to throw int() around those variables before passing them into the SQL.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197717|0 |1 is obsolete| | Attachment #197718|0 |1 is obsolete| | Attachment #197719|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #13 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198255 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198255&action=edit Bug 41918: Add method to see running report queries by user and id Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198256 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198256&action=edit Bug 41918: Prevent a given librarian from running the same report multiple times simultaneously Test Plan: 1) create a long running report such as: SELECT COUNT(*) FROM items i1 JOIN items i2 JOIN items i3 JOIN items i4 JOIN items i5 JOIN items i6 2) Run this report multiple times 3) Note the processes are running via SHOW FULL PROCESSLIST from koha-mysql 4) Kill those processes 5) Apply this patch 6) Restart all the things! 7) Try to run the report in two tabs, note the second tab opened says the report is already running! Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198257 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198257&action=edit Bug 41918: Add ability to disable or change limit via config xml This commit add the option duplicate_running_reports_per_user_limit to koha-conf.xml. If this option is empty or 0, the feature is disabled. If this option is enabled, Koha will limit the number of times a given report can be run of a single Koha user. Test Plan: 1) Apply this patch 2) Set duplicate_running_reports_per_user_limit to 1, verify previous behavior is maintained 2) Set duplicate_running_reports_per_user_limit to 0, verify the feature is disabled 3) Set duplicate_running_reports_per_user_limit to 2, verify Koha limits the number of duplicates of the report to the number specified in duplicate_running_reports_per_user_limit Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #16 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198258 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198258&action=edit Bug 41918: (QA follow-up) Refactor Koha::Reports->running - Fix POD synopsis: invalid syntax and wrong field (userid vs borrowernumber) replaced with a correct example and parameter list. - Build the WHERE clause with parameterised bindings via $schema->storage->dbh, removing string interpolation of $user_id / $report_id into LIKE patterns and the regex match. IDs are formatted with %d so non-integer input becomes 0 and cannot inject regex/LIKE metacharacters. - Return a Koha::Reports resultset instead of a list of MySQL process ids. The method name now matches what it returns and callers can use the standard ->count / iteration interface. The single in-tree caller in reports/guided_reports.pl is updated; the template error key is simplified from duplicate_running_report_ids to a boolean duplicate_running_report (the ids were never displayed). Test plan: 1) Apply the previous Bug 41918 patches and this follow-up. 2) Set duplicate_running_reports_per_user_limit to 1 in koha-conf.xml. 3) Restart all the things. 4) Open a long-running saved report in two tabs and run it from both. 5) Verify the second tab shows "This report is already running..." 6) Set the limit to 0 and confirm both tabs run normally. 7) prove t/db_dependent/Reports.t still passes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #17 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198259 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198259&action=edit Bug 41918: (QA follow-up) Update prep_report tests for new SQL comment The first patch in this series changed the SQL comment appended by Koha::Report::prep_report from /* saved_sql.id: <id> */ to /* { saved_sql.id: <id> } { user_id: <user_id> } */ but did not update the existing assertions in t/db_dependent/Koha/Reports.t, leaving three subtest failures in the prep_report subtest. Refresh the expected strings to match the new format and pick up the userenv-derived user_id (or 0 when no userenv is in scope, matching prep_report's fallback). Test plan: 1) Apply this follow-up. 2) prove t/db_dependent/Koha/Reports.t 3) Verify all 10 tests pass (previously 3 subtests of prep_report failed). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #18 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198260 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198260&action=edit Bug 41918: (QA follow-up) Degrade gracefully without PROCESS privilege Koha::Reports->running queries information_schema.processlist. A MySQL user without the PROCESS privilege still sees its own threads, but if the privilege check fails entirely (or the engine returns an unexpected error) the previous code would let the DBI exception bubble up and abort the calling report run. Wrap the selectall in eval and on failure log via Koha::Logger and treat the result as empty. This means a misconfigured database degrades to "no enforcement" rather than to "no reports can be run", matching the principle that the duplicate-running guard is a safeguard, not a critical path. The POD now documents this fallback so future readers know the empty resultset has two possible meanings: nothing is currently running, or we could not look. The Koha::Logger->warn ensures the latter is still observable in the configured log destination. Test plan: 1) prove t/db_dependent/Koha/Reports.t still passes. 2) Optional: revoke SELECT on information_schema.processlist for the koha DB user, run a report, observe a warning in the logs and the report still executes (with no duplicate enforcement). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #19 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198261 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198261&action=edit Bug 41918: (QA follow-up) Add unit tests for Koha::Reports->running Adds a 'running' subtest covering the new method: - Returns a Koha::Reports resultset (object identity, not opaque ids). - Returns an empty resultset when nothing is in flight, and when the user_id / report_id filter does not match any process. - Parses saved_sql.id markers out of synthetic processlist rows and resolves them to real Koha::Report objects, ignoring rows without the marker. - Sends parameterised LIKE binds for user_id (proves the WHERE clause is built without string interpolation). - Falls back to an empty resultset and does not propagate the exception when the underlying selectall_arrayref call dies, eg. when the database user lacks PROCESS privilege. To make the failure path mockable without trying to mock DBI::db (which is not loadable as a separate file), the dbh selectall is extracted into a tiny Koha::Reports::_processlist_rows helper so Test::MockModule can replace it cleanly. The helper is otherwise behaviour-preserving. Test plan: 1) prove t/db_dependent/Koha/Reports.t -> 11 tests, all pass. 2) Existing prep_report subtest still passes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #20 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198262 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198262&action=edit Bug 41918: (QA follow-up) Document SQL marker contract and TOCTOU window Two small documentation improvements following on from the duplicate- running guard: - Koha::Report::prep_report now spells out the tracking-comment format in its POD. Koha::Reports->running parses this comment, and external log/audit tooling that consumes Koha SQL relied on the previous format (/* saved_sql.id: <id> */). Calling out the new format explicitly makes the public contract visible to anyone reading the POD or grepping for it, and warns future contributors that changing the wrapper will break running() too. - reports/guided_reports.pl gets an inline comment explaining the TOCTOU window between the count check and the actual report execution. The window is intentional - the guard exists to stop accidental double-clicks and two-tab submissions, not to be a serializing lock - so worst case is one extra concurrent run, which is acceptable and cheaper than the alternative. No behaviour change. Test plan: 1) prove t/db_dependent/Koha/Reports.t still passes. 2) perldoc Koha::Report -> prep_report section now mentions the saved_sql.id / user_id wrapper format. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #21 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198263 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198263&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #22 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- I'd like Kyles feedback on the follow-ups, but if he agrees then I think we're good to set PQA here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #23 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize (ashimema) from comment #18)
Created attachment 198260 [details] [review] Bug 41918: (QA follow-up) Degrade gracefully without PROCESS privilege
Koha::Reports->running queries information_schema.processlist. A MySQL user without the PROCESS privilege still sees its own threads, but if the privilege check fails entirely (or the engine returns an unexpected error) the previous code would let the DBI exception bubble up and abort the calling report run.
I was thinking that if they use a database user that is too privileged it could see other Koha database's reports as well, so it might be a good idea to narrow the query even further. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #24 from David Cook <dcook@prosentient.com.au> --- I haven't delved deeply into this one... but what about the SVC endpoint for unauthenticated users? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #25 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198273 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198273&action=edit Bug 41918: (QA follow-up) Scope processlist query to the current DB user Koha::Reports->running queries information_schema.processlist. Without the PROCESS privilege MariaDB/MySQL already restrict the view to the caller's own threads, so the previous code worked correctly on stock deployments. However, if a site has granted broader privileges (eg. a shared MariaDB server where the Koha DB user happens to also have PROCESS, or a custom role) the query would happily count threads belonging to other database users - another Koha instance, a reporting tool, or any unrelated application sharing the server. Add an explicit user = SUBSTRING_INDEX(CURRENT_USER(), '@', 1) clause so the query is always scoped to the connection's MySQL user, regardless of what privileges that user has been granted. The processlist.user column holds just the username with no host, so we strip the '@host' off CURRENT_USER() before comparing. The new clause has no bind parameters - it is a constant per connection - so the existing parameterised-bind assertions in the running subtest still hold. A small additional assertion verifies the clause is present in the SQL text. Test plan: 1) prove t/db_dependent/Koha/Reports.t (now 12 tests, all pass). 2) Optional: in a multi-tenant MariaDB, grant PROCESS to the Koha DB user, start a long query as a different DB user with the marker /* { saved_sql.id: 1 } { user_id: 1 } */ in its info, then call Koha::Reports->running. Confirm the foreign thread is NOT counted. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #26 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Thanks David.. I thought about the possability of users with too many priviledges and then promptly forgot to impliment it.. added in a follow-up now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #27 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- (In reply to David Cook from comment #24)
I haven't delved deeply into this one... but what about the SVC endpoint for unauthenticated users?
That is covered in my follow-ups.. in particular `Enforce duplicate-running limit in the model` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #28 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- I was tempted to move the check into the 'execute_query' from it's current location in 'prep_report'.. open to opinion there. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #29 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 198274 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198274&action=edit Bug 41918: (QA follow-up) Catch DuplicateRunning in op=export path The CSV/tab export branch (op=export) of reports/guided_reports.pl calls Koha::Report::prep_report just like the op=run branch and the two svc/report endpoints, but unlike the others it had no eval/catch around the call. The QA follow-ups that pushed enforcement into the model layer therefore left this one path emitting a 500 (uncaught Koha::Exceptions::Report::DuplicateRunning) when the configured duplicate-running limit is exceeded. Wrap the call in the same pattern as svc/report: on DuplicateRunning return HTTP 429 Too Many Requests with a plain-text body explaining the situation. Plain text rather than the staff template because op=export sets file-download headers and the user is expecting either the file or an error - we don't have a sensible way to fall back into the report-run template from this branch. The CLI runners (misc/cronjobs/runreport.pl, misc/export_records.pl) still bypass prep_report and therefore the limit, which is intended: scheduled / batch jobs should not be subject to user-quota enforcement. Test plan: 1) Set duplicate_running_reports_per_user_limit=1 in koha-conf.xml, restart_all. 2) Start a long-running report from the staff UI. 3) While it is running, navigate to op=export with the same report id (eg. via the "Export" button). 4) Before the patch: 500 with a stack trace in the logs. After the patch: 429 with body "This report is already running. Please wait for it to finish before running it again." 5) Set the limit back to 0 and confirm exports work normally again. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_26_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #30 from Jonathan Druart <jonathan.druart@gmail.com> --- Shouldn't we use background jobs instead? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #31 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #30)
Shouldn't we use background jobs instead?
It's certainly an option. I suppose some things to keep in mind for that are: 1. We'd have to store the results which would increase the size of the database (But I suppose cleanup_database.pl could clear out report results) 2. Depending on how many workers there are, it might take a while for a report to run. (But that isn't necessarily a bad thing. That could actually improve Koha performance overall.) 3. Report results would get stale. We'd need to stress that the report results were true at the time it was run rather than the time it was viewed. 4. We'd have to make exceptions for the SVC endpoint (although that endpoint needs a massive overhaul anyway...) Probably some other things to keep in mind that I can't think of right now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #32 from David Cook <dcook@prosentient.com.au> --- Thinking about this again... and still not loving the implementation. It feels hacky looking for a comment in the process list. This is a good use case for advisory locking, and it looks like MySQL/MariaDB have support for this kind of cooperative distributed advisory locking: https://dev.mysql.com/doc/refman/8.4/en/locking-functions.html It would be as easy as using these functions: IS_FREE_LOCK("koha_library.report:1"); GET_LOCK("koha_library.report:1",10); The lock is freed when it's released by the process that has the lock or when that connection dies (so if it errored out there's automatic cleanup). It would be pretty simple, and it would allow us to limit running a report so that only one process could do it at a time. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #33 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #30)
Shouldn't we use background jobs instead?
yes, that's the end of this chain: https://bugs.koha-community.org/bugzilla3/showdependencygraph.cgi?id=41919 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #34 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 198750 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198750&action=edit Bug 41918: (RM follow-up) Add POD Patch from commit ccc9192 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 --- Comment #35 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 26.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_26_05_candidate | CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Jacob O'Mara <jacob.omara@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Needs documenting --- Comment #36 from Jacob O'Mara <jacob.omara@openfifth.co.uk> --- This will not be backported to 25.11.x due to it being deemed an enhancement or a dependancy not being met -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41918 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43016 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43016 [Bug 43016] [OMNIBUS] Server resource protection -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org