From 22d9133268b90ceabffa51fca072fd2828de9e25 Mon Sep 17 00:00:00 2001 From: Andrew Moore Date: Fri, 25 Jul 2008 11:55:12 -0500 Subject: [PATCH] Bug 1953 [2/3]: refactoring SQL in C4::Items::GetItemsForInventory to use placeholders The SQL in C4::Items::GetItemsForInventory wasn't using placeholders and bind parameters, possibly leaving itself open ot SQL injection attacks. This patch changes that. I've also incliuded a test module for C4::items::GetItemsForInventory. Signed-off-by: Joshua Ferraro --- C4/Items.pm | 63 +++++++------- t/lib/KohaTest/Items/GetItemsForInventory.pm | 123 +++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 30 deletions(-) create mode 100644 t/lib/KohaTest/Items/GetItemsForInventory.pm diff --git a/C4/Items.pm b/C4/Items.pm index d77c654989..cbbfc0dd76 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -933,39 +933,42 @@ offset & size can be used to retrieve only a part of the whole listing (defaut b sub GetItemsForInventory { my ( $minlocation, $maxlocation,$location, $itemtype, $datelastseen, $branch, $offset, $size ) = @_; my $dbh = C4::Context->dbh; - my $sth; + + my $query = <<'END_SQL'; +SELECT itemnumber, barcode, itemcallnumber, title, author, biblio.biblionumber, datelastseen +FROM items + LEFT JOIN biblio ON items.biblionumber = biblio.biblionumber + LEFT JOIN biblioitems on items.biblionumber = biblioitems.biblionumber +WHERE itemcallnumber >= ? + AND itemcallnumber <= ? +END_SQL + my @bind_params = ( $minlocation, $maxlocation ); + if ($datelastseen) { - $datelastseen=format_date_in_iso($datelastseen); - my $query = - "SELECT itemnumber,barcode,itemcallnumber,title,author,biblio.biblionumber,datelastseen - FROM items - LEFT JOIN biblio ON items.biblionumber=biblio.biblionumber - LEFT JOIN biblioitems on items.biblionumber=biblioitems.biblionumber - WHERE itemcallnumber>= ? - AND itemcallnumber <=? - AND (datelastseen< ? OR datelastseen IS NULL)"; - $query.= " AND items.location=".$dbh->quote($location) if $location; - $query.= " AND items.homebranch=".$dbh->quote($branch) if $branch; - $query.= " AND biblioitems.itemtype=".$dbh->quote($itemtype) if $itemtype; - $query .= " ORDER BY itemcallnumber,title"; - $sth = $dbh->prepare($query); - $sth->execute( $minlocation, $maxlocation, $datelastseen ); + $datelastseen = format_date_in_iso($datelastseen); + $query .= ' AND (datelastseen < ? OR datelastseen IS NULL) '; + push @bind_params, $datelastseen; } - else { - my $query =" - SELECT itemnumber,barcode,itemcallnumber,biblio.biblionumber,title,author,datelastseen - FROM items - LEFT JOIN biblio ON items.biblionumber=biblio.biblionumber - LEFT JOIN biblioitems on items.biblionumber=biblioitems.biblionumber - WHERE itemcallnumber>= ? - AND itemcallnumber <=?"; - $query.= " AND items.location=".$dbh->quote($location) if $location; - $query.= " AND items.homebranch=".$dbh->quote($branch) if $branch; - $query.= " AND biblioitems.itemtype=".$dbh->quote($itemtype) if $itemtype; - $query .= " ORDER BY itemcallnumber,title"; - $sth = $dbh->prepare($query); - $sth->execute( $minlocation, $maxlocation ); + + if ( $location ) { + $query.= ' AND items.location = ? '; + push @bind_params, $location; + } + + if ( $branch ) { + $query.= ' AND items.homebranch = ? '; + push @bind_params, $branch; } + + if ( $itemtype ) { + $query.= ' AND biblioitems.itemtype = ? '; + push @bind_params, $itemtype; + } + + $query .= ' ORDER BY itemcallnumber, title'; + my $sth = $dbh->prepare($query); + $sth->execute( @bind_params ); + my @results; $size--; while ( my $row = $sth->fetchrow_hashref ) { diff --git a/t/lib/KohaTest/Items/GetItemsForInventory.pm b/t/lib/KohaTest/Items/GetItemsForInventory.pm new file mode 100644 index 0000000000..03cb4a1907 --- /dev/null +++ b/t/lib/KohaTest/Items/GetItemsForInventory.pm @@ -0,0 +1,123 @@ +package KohaTest::Items::GetItemsForInventory; +use base qw( KohaTest::Items ); + +use strict; +use warnings; + +use Test::More; + +use C4::Items; + +=head2 STARTUP METHODS + +These get run once, before the main test methods in this module + +=cut + +=head2 startup_90_add_item_get_callnumber + +=cut + +sub startup_90_add_item_get_callnumber : Test( startup => 13 ) { + my $self = shift; + + $self->add_biblios( add_items => 1 ); + + ok( $self->{'biblios'}, 'An item has been aded' ) + or diag( Data::Dumper->Dump( [ $self->{'biblios'} ], ['biblios'] ) ); + + my @biblioitems = C4::Biblio::GetBiblioItemByBiblioNumber( $self->{'biblios'}[0] ); + ok( $biblioitems[0]->{'biblioitemnumber'}, '...and it has a biblioitemnumber' ) + or diag( Data::Dumper->Dump( [ \@biblioitems ], ['biblioitems'] ) ); + + my $items_info = GetItemsByBiblioitemnumber( $biblioitems[0]->{'biblioitemnumber'} ); + isa_ok( $items_info, 'ARRAY', '...and we can search with that biblioitemnumber' ) + or diag( Data::Dumper->Dump( [$items_info], ['items_info'] ) ); + cmp_ok( scalar @$items_info, '>', 0, '...and we can find at least one item with that biblioitemnumber' ); + + my $item_info = $items_info->[0]; + ok( $item_info->{'itemcallnumber'}, '...and the item we found has a call number: ' . $item_info->{'itemcallnumber'} ) + or diag( Data::Dumper->Dump( [$item_info], ['item_info'] ) ); + + $self->{'callnumber'} = $item_info->{'itemcallnumber'}; +} + + +=head2 TEST METHODS + +standard test methods + +=head3 missing_parameters + +the minlocation and maxlocation parameters are required. If they are +not provided, this method should somehow complain, such as returning +undef or emitina warning or something. + +=cut + +sub missing_parameters : Test( 1 ) { + my $self = shift; + local $TODO = 'GetItemsForInventory should fail when missing required parameters'; + + my $items = C4::Items::GetItemsForInventory(); + ok( not $items, 'GetItemsForInventory fails when parameters are missing' ) + or diag( Data::Dumper->Dump( [ $items ], [ 'items' ] ) ); +} + +=head3 basic_usage + + +=cut + +sub basic_usage : Test( 4 ) { + my $self = shift; + + ok( $self->{'callnumber'}, 'we have a call number to search for: ' . $self->{'callnumber'} ); + my $items = C4::Items::GetItemsForInventory( $self->{'callnumber'}, $self->{'callnumber'} ); + isa_ok( $items, 'ARRAY', 'We were able to call GetItemsForInventory with our call number' ); + is( scalar @$items, 1, '...and we found only one item' ); + my $our_item = $items->[0]; + is( $our_item->{'itemnumber'}, $self->{'biblios'}[0], '...and the item we found has the right itemnumber' ); + + diag( Data::Dumper->Dump( [$items], ['items'] ) ); +} + +=head3 date_last_seen + + +=cut + +sub date_last_seen : Test( 6 ) { + my $self = shift; + + ok( $self->{'callnumber'}, 'we have a call number to search for: ' . $self->{'callnumber'} ); + + my $items = C4::Items::GetItemsForInventory( + $self->{'callnumber'}, # minlocation + $self->{'callnumber'}, # maxlocation + undef, # location + undef, # itemtype + C4::Dates->new( $self->tomorrow(), 'iso' )->output, # datelastseen + ); + + isa_ok( $items, 'ARRAY', 'We were able to call GetItemsForInventory with our call number' ); + is( scalar @$items, 1, '...and we found only one item' ); + my $our_item = $items->[0]; + is( $our_item->{'itemnumber'}, $self->{'biblios'}[0], '...and the item we found has the right itemnumber' ); + + # give a datelastseen of yesterday, and we should not get our item. + $items = C4::Items::GetItemsForInventory( + $self->{'callnumber'}, # minlocation + $self->{'callnumber'}, # maxlocation + undef, # location + undef, # itemtype + C4::Dates->new( $self->yesterday(), 'iso' )->output, # datelastseen + ); + + isa_ok( $items, 'ARRAY', 'We were able to call GetItemsForInventory with our call number' ); + is( scalar @$items, 0, '...and we found no items' ); + +} + + +1; -- 2.11.0