Bug 24695 - Improve SQL report validation
Summary: Improve SQL report validation
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: paxed
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 21215
  Show dependency treegraph
 
Reported: 2020-02-20 09:38 UTC by paxed
Modified: 2021-04-19 12:47 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 24695: Improve SQL report validation (4.83 KB, patch)
2020-02-20 09:41 UTC, paxed
Details | Diff | Splinter Review
Bug 24695: Improve SQL report validation (4.87 KB, patch)
2020-02-25 23:32 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 24695: Move to Koha::Report->is_sql_valid (6.11 KB, patch)
2020-02-27 09:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24695: Improve SQL report validation (4.74 KB, patch)
2021-02-25 17:25 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24695: Move to Koha::Report->is_sql_valid (6.16 KB, patch)
2021-02-25 17:25 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24695: Improve SQL report validation (4.81 KB, patch)
2021-04-09 16:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24695: Move to Koha::Report->is_sql_valid (6.21 KB, patch)
2021-04-09 16:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24695: (QA follow-up) Fix number of tests (710 bytes, patch)
2021-04-09 16:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24695: (QA follow-up) Fix POD (1.09 KB, patch)
2021-04-09 16:44 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2020-02-20 09:38:17 UTC
The saved SQL report code validates the SQL in multiple places: when saving, when updating, and when executing the query. Move the validation code into Koha::Reports, and write tests for it.
Comment 1 paxed 2020-02-20 09:41:36 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2020-02-20 13:32:14 UTC
(In reply to paxed from comment #1)
> Created attachment 99305 [details] [review] [review]
> Bug 24695: Improve SQL report validation

Works well, no errors, test pass, but...

> 5) Edit a save_sql in the database, changing it to eg.
>    "drop borrowers", and try to execute it (fails)

ended with an empty borrowers table :(
(I wrote 'delete from borrowers')

Works this for you?
Comment 3 paxed 2020-02-21 05:28:13 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #2)
> (In reply to paxed from comment #1)
> > Created attachment 99305 [details] [review] [review] [review]
> > Bug 24695: Improve SQL report validation
> 
> Works well, no errors, test pass, but...
> 
> > 5) Edit a save_sql in the database, changing it to eg.
> >    "drop borrowers", and try to execute it (fails)
> 
> ended with an empty borrowers table :(
> (I wrote 'delete from borrowers')
> 
> Works this for you?

update saved_sql set savedsql='DROP borrowers';

then try to run the report, it'll cause an error. Ideally, this should be changed to produce an actual koha page with an explanation instead, but that is outside the scope of this bug, and behaves the same as before this patch.
Comment 4 Martin Renvoize 2020-02-25 17:35:42 UTC
Great to see some cleanup going on around this area :)
Comment 5 Bernardo Gonzalez Kriegel 2020-02-25 23:32:21 UTC
Created attachment 99625 [details] [review]
Bug 24695: Improve SQL report validation

The saved SQL report code validates the SQL in multiple places:
when saving, when updating, and when executing the query.
Move the validation code into Koha::Reports, and write tests for it.

Test plan:
1) Apply patch
2) Create a new valid SQL report, save it (success)
3) Create a new illegal SQL report, try to save (fails)
4) Update already saved SQL report by adding one of
   the forbidden words, eg. delete or drop (saving will fail)
5) Edit a save_sql in the database, changing it to eg.
   "drop borrowers", and try to execute it (fails)
6) Prove t/db_dependent/Koha/Reports.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described, no qa errors.
Comment 6 Jonathan Druart 2020-02-26 15:15:16 UTC
Why in Koha::Reports and not Koha::Report?

I think this code would make sense here:
  $report->store if $report->is_sql_valid;
Comment 7 Jonathan Druart 2020-02-26 15:16:59 UTC
It could even prevent the store in an overridden ->store method. And throw an exception if not valid.
Comment 8 paxed 2020-02-27 07:26:28 UTC
(In reply to Jonathan Druart from comment #7)
> It could even prevent the store in an overridden ->store method. And throw
> an exception if not valid.

I don't know how to do it that way off the top of my head, so I'd have to spend several days on this, and I don't have that kind of time to spend.
Comment 9 Jonathan Druart 2020-02-27 09:31:48 UTC
Created attachment 99694 [details] [review]
Bug 24695: Move to Koha::Report->is_sql_valid
Comment 10 Jonathan Druart 2020-02-27 09:33:12 UTC
(In reply to paxed from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > It could even prevent the store in an overridden ->store method. And throw
> > an exception if not valid.
> 
> I don't know how to do it that way off the top of my head, so I'd have to
> spend several days on this, and I don't have that kind of time to spend.

Dropping it for now as we do not use Koha::Report->store yet to insert them.

What about this patch?
Comment 11 Séverine Queune 2020-10-13 15:22:52 UTC
On step 3, the SQL content is emptied after the warning alert.
Is it the expected behaviour ?
I use a lot this tool, and I think it will be quite frustating after a mistake to lose all the SQL I just (sometimes painfully...) write.

All the other steps are ok for me !
Comment 12 Jonathan Druart 2021-02-10 07:30:38 UTC
Hi Séverine,
If I click the "Return to previous page" I get the form prefilled with what I entered. So what you described seems to have been fixed now :)
Comment 13 Owen Leonard 2021-02-25 17:25:18 UTC
Created attachment 117352 [details] [review]
Bug 24695: Improve SQL report validation

The saved SQL report code validates the SQL in multiple places:
when saving, when updating, and when executing the query.
Move the validation code into Koha::Reports, and write tests for it.

Test plan:
1) Apply patch
2) Create a new valid SQL report, save it (success)
3) Create a new illegal SQL report, try to save (fails)
4) Update already saved SQL report by adding one of
   the forbidden words, eg. delete or drop (saving will fail)
5) Edit a save_sql in the database, changing it to eg.
   "drop borrowers", and try to execute it (fails)
6) Prove t/db_dependent/Koha/Reports.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described, no qa errors.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Owen Leonard 2021-02-25 17:25:22 UTC
Created attachment 117353 [details] [review]
Bug 24695: Move to Koha::Report->is_sql_valid

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Kyle M Hall 2021-04-09 16:44:01 UTC
Created attachment 119421 [details] [review]
Bug 24695: Improve SQL report validation

The saved SQL report code validates the SQL in multiple places:
when saving, when updating, and when executing the query.
Move the validation code into Koha::Reports, and write tests for it.

Test plan:
1) Apply patch
2) Create a new valid SQL report, save it (success)
3) Create a new illegal SQL report, try to save (fails)
4) Update already saved SQL report by adding one of
   the forbidden words, eg. delete or drop (saving will fail)
5) Edit a save_sql in the database, changing it to eg.
   "drop borrowers", and try to execute it (fails)
6) Prove t/db_dependent/Koha/Reports.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described, no qa errors.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2021-04-09 16:44:12 UTC
Created attachment 119422 [details] [review]
Bug 24695: Move to Koha::Report->is_sql_valid

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2021-04-09 16:44:15 UTC
Created attachment 119423 [details] [review]
Bug 24695: (QA follow-up) Fix number of tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2021-04-09 16:44:19 UTC
Created attachment 119424 [details] [review]
Bug 24695: (QA follow-up) Fix POD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Jonathan Druart 2021-04-12 13:32:18 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 20 Fridolin Somers 2021-04-19 12:47:53 UTC
Enhancement not pushed to 20.11.x