From 8d756ca0b1c33e21cf781bd0bb4f92edec4c2806 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Fri, 29 May 2015 10:56:12 +0200 Subject: [PATCH] Bug 14296: Introduce upload settings, check size and translate alerts Content-Type: text/plain; charset=utf-8 This patch includes: [1] Add sub MaxUploadSize to module and add unit test. Note that normally the table will still be empty, but a large fallback is used. [2] Check the max upload size for the user when submitting the form and again when processing it. [3] Make it possible to translate some alerts in upload.tt. Also it moves the input[type=file] control to the template. Test plan: [1] Upload a file. No restrictions. [2] Run UploadedFiles.t. [3] Add a record (sorry, interface is another report) via mysql into upload_settings: type=filesize, count1=1000. [4] Upload a file larger than 1000 bytes. Should not be possible. [5] Upload a smaller file. [6] Bonus: I tested the second size check by adding a return true in the javascript validation after the first size check. --- C4/UploadedFiles.pm | 37 ++++++++++++-- cataloguing/value_builder/upload.pl | 14 ++---- .../en/modules/cataloguing/value_builder/upload.tt | 51 ++++++++++---------- t/db_dependent/UploadedFiles.t | 42 +++++++++++++++- 4 files changed, 105 insertions(+), 39 deletions(-) diff --git a/C4/UploadedFiles.pm b/C4/UploadedFiles.pm index d8a5be4..6fdeae0 100644 --- a/C4/UploadedFiles.pm +++ b/C4/UploadedFiles.pm @@ -59,6 +59,8 @@ use File::Basename; use C4::Context; +use constant MAX_DEFAULT_UPLOAD => 10000000; # relatively large fallback; + sub _get_file_path { my ($id, $dirname, $filename) = @_; @@ -142,12 +144,22 @@ sub UploadFile { return; } + my $ue = C4::Context->userenv; + my $owner = $ue? $ue->{number}: undef; + my $max = C4::UploadedFiles::MaxUploadSize( $owner ); + my $buffer; my $data = ''; - while($handle->read($buffer, 1024)) { + while( $handle->read($buffer, 1024) && length($data) <= $max ) { $data .= $buffer; } $handle->close; + if( length($data) > $max ) { + # double check actually (see js); normally not possible, so we just + # rely on a general error message and a warn + warn "UploadFile: upload size exceeded after submit!"; + return; + } my $sha = new Digest::SHA; $sha->add($data); @@ -180,8 +192,6 @@ sub UploadFile { VALUES (?,?, ?, ?, ?); }; my $sth = $dbh->prepare($query); - my $ue = C4::Context->userenv; - my $owner = $ue? $ue->{number}: undef; if( $sth->execute($id, $filename, $dir, length($data), $owner ) ) { return $id; } @@ -335,4 +345,25 @@ sub httpheaders { '-attachment' => $file, ); } +=head2 MaxUploadSize + + Returns max upload size for borrower or if the borrower has no explicit + user limit, the default max. If no records are found, falls back to a + constant. + +=cut + +sub MaxUploadSize { + my ( $user ) = @_; + my $dbh = C4::Context->dbh; + my $query = qq| +SELECT count1 FROM upload_settings +WHERE (borrowernumber=? OR borrowernumber IS NULL) AND + type='filesize' +ORDER BY IFNULL(borrowernumber, 0) DESC; + |; + my ( $max ) = $dbh->selectrow_array( $query, undef, ( $user ) ); + return $max // MAX_DEFAULT_UPLOAD; +} + 1; diff --git a/cataloguing/value_builder/upload.pl b/cataloguing/value_builder/upload.pl index ee2145e..01428fb 100755 --- a/cataloguing/value_builder/upload.pl +++ b/cataloguing/value_builder/upload.pl @@ -90,10 +90,9 @@ my $launcher = sub { $id = C4::UploadedFiles::UploadFile($uploaded_file, $dir, $fh->handle); my $OPACBaseURL = C4::Context->preference('OPACBaseURL') // ''; $OPACBaseURL =~ s#/$##; - if (!$OPACBaseURL) { - $template->param(MissingURL => 1); - } - if($id && $OPACBaseURL) { + if( !$OPACBaseURL ) { + $template->param( MissingURL => 1 ); + } elsif( $id ) { my $return = "$OPACBaseURL/cgi-bin/koha/opac-retrieve-file.pl?id=$id"; $template->param( success => 1, @@ -116,11 +115,6 @@ my $launcher = sub { } else { my $upload_path = C4::Context->config('upload_path'); if ($upload_path) { - my $filefield = CGI::filefield( - -name => 'uploaded_file', - -size => 50, - ); - my $dirs_tree = [ { name => '/', value => '/', @@ -129,7 +123,7 @@ my $launcher = sub { $template->param( dirs_tree => $dirs_tree, - filefield => $filefield, + maxfilesize => C4::UploadedFiles::MaxUploadSize($loggedinuser), ); } else { $template->param( error_upload_path_not_configured => 1 ); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt index e08e424..2a74817 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt @@ -7,32 +7,33 @@ @@ -102,9 +103,9 @@ [% IF (dangling) %]

Error: The URL has no file to retrieve.

[% END %] -

Please select the file to upload :

-
- [% filefield %] +

Please select the file to upload:

+ +

Choose where to upload the file

[% INCLUDE list_dirs dirs = dirs_tree %] diff --git a/t/db_dependent/UploadedFiles.t b/t/db_dependent/UploadedFiles.t index f0594c5..1d85970 100644 --- a/t/db_dependent/UploadedFiles.t +++ b/t/db_dependent/UploadedFiles.t @@ -3,7 +3,7 @@ use Modern::Perl; use File::Temp qw/tempdir/; use Test::CGI::Multipart; -use Test::More tests => 20; +use Test::More tests => 21; use Test::Warn; use t::lib::Mocks; @@ -11,6 +11,10 @@ use t::lib::Mocks; use C4::Context; use C4::UploadedFiles; +my $dbh = C4::Context->dbh; +$dbh->{AutoCommit} = 0; +$dbh->{RaiseError} = 1; + # This simulates a multipart POST request with a file upload. my $tcm = new Test::CGI::Multipart; $tcm->upload_file( @@ -70,3 +74,39 @@ mkdir $temp2.'/A/AA'; mkdir $temp2.'/A/BB'; $a= C4::UploadedFiles::finddirs( $temp2 ); is( @{$a->[0]->{dirs}} == 2, 1, 'Finddir returns subdirectories' ); + +#tests for MaxUploadSize +subtest 'Tests for MaxUploadSize' => sub { + test_max(); +}; + +#end of game +$dbh->rollback; + +sub test_max { + $dbh->do("DELETE FROM upload_settings"); + my $max = C4::UploadedFiles::MaxUploadSize(); + is( $max, 10000000, 'MaxUploadSize is equal to fallback' ); + + $dbh->do("INSERT INTO upload_settings (type, count1) VALUES (?, ?)", + undef, ( 'filesize', 500 )); + $max = C4::UploadedFiles::MaxUploadSize(); + is( $max, 500, 'MaxUploadSize is equal to default' ); + + my $borr = $dbh->selectall_arrayref( + 'SELECT borrowernumber FROM borrowers LIMIT 2' ); + if( @$borr < 2 ) { + done_testing(2); + return; + } + my $b1 = $borr->[0]->[0]; + $dbh->do("INSERT INTO upload_settings (borrowernumber, type, count1) + VALUES (?, ?, ?)", undef, ( $b1, 'filesize', 1500 )); + $max = C4::UploadedFiles::MaxUploadSize($b1); + is( $max, 1500, 'MaxUploadSize is equal to user limit' ); + + my $b2 = $borr->[1]->[0]; + $max = C4::UploadedFiles::MaxUploadSize($b2); + is( $max, 500, 'MaxUploadSize is still equal to default for user 2' ); + done_testing(4); +} -- 1.7.10.4