Bug 19230 - Warn when deleting a course in course reserves
Summary: Warn when deleting a course in course reserves
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Course reserves (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-31 21:40 UTC by Aleisha Amohia
Modified: 2018-06-04 20:15 UTC (History)
4 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19230: Preventing warn when deleting course (1.05 KB, patch)
2017-08-31 21:49 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 19230: Preventing warn when deleting course (1.12 KB, patch)
2017-10-07 13:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19230: Preventing warn when deleting course (1.12 KB, patch)
2017-12-20 07:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19230: Preventing warn when deleting course (3.25 KB, patch)
2017-12-21 01:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 19230: Preventing warn when deleting course (3.30 KB, patch)
2018-01-19 20:46 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 19230: Preventing warn when deleting course (3.39 KB, patch)
2018-01-26 07:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19230: (follow-up) Preventing warn when deleting course (1.27 KB, patch)
2018-01-30 17:07 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2017-08-31 21:40:36 UTC
To reproduce:
1) Ensure UseCourseReserves is enabled
2) Go to Course Reserves, create a course
3) Delete course
4) Notice warn
CGI::param called in list context from package CGI::Compile::ROOT::home_vagrant_kohaclone_course_reserves_mod_course_2epl line 43, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
Comment 1 Aleisha Amohia 2017-08-31 21:49:20 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2017-10-07 13:04:14 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2017-12-19 15:26:54 UTC
I think it would be better to assign a $course_id variable and reuse it.
Something like:

modified: course_reserves/mod_course.pl
@ mod_course.pl:41 @ my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
 );

 my $action = $cgi->param('action') || '';
+my $course_id = $cgi->param('course_id');

 if ( $action eq 'del' ) {
-    DelCourse( $cgi->param('course_id') );
+    DelCourse( $course_id );
     print $cgi->redirect("/cgi-bin/koha/course_reserves/course-reserves.pl");
 } else {
     my %params;

-    $params{'course_id'} = $cgi->param('course_id')
-      if ( $cgi->param('course_id') );
+    $params{'course_id'} = $course_id;
     $params{'department'}     = $cgi->param('department');
     $params{'course_number'}  = $cgi->param('course_number');
     $params{'section'}        = $cgi->param('section');
@ mod_course.pl:60 @ if ( $action eq 'del' ) {
     $params{'students_count'} = $cgi->param('students_count');
     $params{'enabled'}        = ( $cgi->param('enabled') eq 'on' ) ? 'yes' : 'no';

-    my $course_id = ModCourse(%params);
+    my $new_course_id = ModCourse(%params);

     my @instructors = $cgi->multi_param('instructors');
     ModCourseInstructors(
         mode        => 'replace',
         cardnumbers => \@instructors,
-        course_id   => $course_id
+        course_id   => $new_course_id
     );
-    print $cgi->redirect("/cgi-bin/koha/course_reserves/course-details.pl?course_id=$course_id");
+    print $cgi->redirect("/cgi-bin/koha/course_reserves/course-details.pl?course_id=$new_course_id");
 }

Would it make more sense?
Comment 4 Katrin Fischer 2017-12-20 07:10:41 UTC
Created attachment 69946 [details] [review]
Bug 19230: Preventing warn when deleting course

To test:
1) Ensure UseCourseReserves is enabled
2) Go to Course Reserves, create a course
3) Delete course
4) Notice warn
5) Apply patch
6) Create a course again and delete it
7) Notice no warn

Sponsored-by: Catalyst IT

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 5 Aleisha Amohia 2017-12-21 01:10:07 UTC
(In reply to Jonathan Druart from comment #3)

> Would it make more sense?

hi Jonathan,
Yes I believe your fix makes more sense so I have implemented in the next patch.

When I was testing, I also found a new warn when looking at a course with NO course reserves in it (course_reserves/course-details.pl)
Argument "" isn't numeric in numeric gt (>) at /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt line 219.

Also, when deleting a course with no course reserves, the confirm message is not triggered.

I've added a fix for this also in my next patch.
Comment 6 Aleisha Amohia 2017-12-21 01:11:46 UTC
Created attachment 69985 [details] [review]
Bug 19230: Preventing warn when deleting course

To test:
1) Ensure UseCourseReserves is enabled
2) Go to Course Reserves, create a course
3) When you are taken to the course page, you should notice the
'Argument "" isn't numeric in numeric gt' warn
4) Delete course
5) Notice warn, and that no confirm message popped up
6) Apply patch
7) Create a course again. Confirm the 'Argument "" isn't numeric in
numeric gt' warn did not show
8) Delete the course
9) Notice no warn and the confirm message pops up

Sponsored-by: Catalyst IT
Comment 7 Roch D'Amour 2018-01-19 20:46:42 UTC
Created attachment 70780 [details] [review]
Bug 19230: Preventing warn when deleting course

To test:
1) Ensure UseCourseReserves is enabled
2) Go to Course Reserves, create a course
3) When you are taken to the course page, you should notice the
'Argument "" isn't numeric in numeric gt' warn
4) Delete course
5) Notice warn, and that no confirm message popped up
6) Apply patch
7) Create a course again. Confirm the 'Argument "" isn't numeric in
numeric gt' warn did not show
8) Delete the course
9) Notice no warn and the confirm message pops up

Sponsored-by: Catalyst IT
Signed-off-by: Roch D'Amour <roch.damour@inlibro.com>
Comment 8 Marcel de Rooy 2018-01-26 07:35:57 UTC
Created attachment 70933 [details] [review]
Bug 19230: Preventing warn when deleting course

To test:
1) Ensure UseCourseReserves is enabled
2) Go to Course Reserves, create a course
3) When you are taken to the course page, you should notice the
'Argument "" isn't numeric in numeric gt' warn
4) Delete course
5) Notice warn, and that no confirm message popped up
6) Apply patch
7) Create a course again. Confirm the 'Argument "" isn't numeric in
numeric gt' warn did not show
8) Delete the course
9) Notice no warn and the confirm message pops up

Sponsored-by: Catalyst IT
Signed-off-by: Roch D'Amour <roch.damour@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Jonathan Druart 2018-01-30 17:07:17 UTC
Created attachment 71067 [details] [review]
Bug 19230: (follow-up) Preventing warn when deleting course

Simplify 'unless' statement
Comment 10 Jonathan Druart 2018-01-30 17:25:57 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 11 Nick Clemens 2018-02-12 20:57:16 UTC
Awesome work all!

This patch has been pushed to 17.11.x and will be in 17.11.03