From d09aa1a513d02684306973dfc0c7e616b98341f3 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 14 Mar 2016 13:22:14 +0100 Subject: [PATCH] Bug 14686: Add allows_add_by to check permissions for uploading Content-Type: text/plain; charset=utf-8 The three permissions in tools/upload-file.pl are moved to a custom routine in DBIx's UploadedFile.pm. An additional granular permission upload_general_files is added. (The dbrev patch will contain it too.) At some point in time this could be moved to a Koha::Object class. The routine is tested in Upload.t. Test plan: [1] Run t/db_dependent/Upload.t [2] If you only apply this patch, you can also test uploading with a user that has edit_catalogue but does not have tools or circulate. Upload status should say: Denied. --- Koha/Schema/Result/UploadedFile.pm | 16 ++++++++++- t/db_dependent/Upload.t | 52 +++++++++++++++++++++++++++++++++--- tools/upload-file.pl | 17 +++--------- 3 files changed, 66 insertions(+), 19 deletions(-) diff --git a/Koha/Schema/Result/UploadedFile.pm b/Koha/Schema/Result/UploadedFile.pm index 1eaab71..8e2106b 100644 --- a/Koha/Schema/Result/UploadedFile.pm +++ b/Koha/Schema/Result/UploadedFile.pm @@ -124,5 +124,19 @@ __PACKAGE__->set_primary_key("id"); # DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:Cq/HSuTaBxZH2qSGEddoaQ -# You can replace this text with custom code or comments, and it will be preserved on regeneration +sub allows_add_by { + my ( $self, $userid ) = @_; # do not confuse with borrowernumber + my $flags = [ + { tools => 'upload_general_files' }, + { circulate => 'circulate_remaining_permissions' }, + { tools => 'stage_marc_import' }, + { tools => 'upload_local_cover_images' }, + ]; + require C4::Auth; + foreach( @$flags ) { + return 1 if C4::Auth::haspermission( $userid, $_ ); + } + return; +} + 1; diff --git a/t/db_dependent/Upload.t b/t/db_dependent/Upload.t index 1a273d5..31d8767 100644 --- a/t/db_dependent/Upload.t +++ b/t/db_dependent/Upload.t @@ -2,17 +2,20 @@ use Modern::Perl; use File::Temp qw/ tempdir /; -use Test::More tests => 7; +use Test::More tests => 8; use Test::MockModule; use t::lib::Mocks; +use t::lib::TestBuilder; use C4::Context; +use Koha::Database; use Koha::Upload; +use Koha::Schema::Result::UploadedFile; +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; my $dbh = C4::Context->dbh; -$dbh->{AutoCommit} = 0; -$dbh->{RaiseError} = 1; our $current_upload = 0; our $uploads = [ @@ -81,7 +84,11 @@ subtest 'Test07' => sub { plan tests => 2; test07(); }; -$dbh->rollback; +subtest 'Test08: UploadedFile->allows_add_by' => sub { + plan tests => 4; + test08(); +}; +$schema->storage->txn_rollback; sub test01 { # Delete existing records (for later tests) @@ -169,6 +176,43 @@ sub test07 { #simple test for httpheaders and getCategories is( @$cat >= 1, 1, 'getCategories returned at least one category' ); } +sub test08 { # UploadedFile->allows_add_by + my $builder = t::lib::TestBuilder->new; + my $patron = $builder->build({ + source => 'Borrower', + value => { flags => 0 }, #no permissions + }); + my $patronid = $patron->{borrowernumber}; + is( Koha::Schema::Result::UploadedFile->allows_add_by( $patron->{userid} ), + undef, 'Patron is not allowed to do anything' ); + + # add some permissions: edit_catalogue + my $fl = 2**9; # edit_catalogue + $schema->resultset('Borrower')->find( $patronid )->update({ flags => $fl }); + is( Koha::Schema::Result::UploadedFile->allows_add_by( $patron->{userid} ), + undef, 'Patron is still not allowed to add uploaded files' ); + + # replace flags by all tools + $fl = 2**13; # tools + $schema->resultset('Borrower')->find( $patronid )->update({ flags => $fl }); + is( Koha::Schema::Result::UploadedFile->allows_add_by( $patron->{userid} ), + 1, 'Patron should be allowed now to add uploaded files' ); + + # remove all tools and add upload_general_files only + $fl = 0; # no modules + $schema->resultset('Borrower')->find( $patronid )->update({ flags => $fl }); + $builder->build({ + source => 'UserPermission', + value => { + borrowernumber => $patronid, + module_bit => { module_bit => { flag => 'tools' } }, + code => 'upload_general_files', + }, + }); + is( Koha::Schema::Result::UploadedFile->allows_add_by( $patron->{userid} ), + 1, 'Patron is still allowed to add uploaded files' ); +} + sub newCGI { my ( $class, $hook ) = @_; my $read = 0; diff --git a/tools/upload-file.pl b/tools/upload-file.pl index 1994214..b410934 100755 --- a/tools/upload-file.pl +++ b/tools/upload-file.pl @@ -28,6 +28,7 @@ use URI::Escape; use C4::Context; use C4::Auth qw/check_cookie_auth haspermission/; use Koha::Upload; +use Koha::Schema::Result::UploadedFile; # upload-file.pl must authenticate the user # before processing the POST request, @@ -37,25 +38,13 @@ use Koha::Upload; # requires that the session cookie already # has been created. -my $flags_required = [ - {circulate => 'circulate_remaining_permissions'}, - {tools => 'stage_marc_import'}, - {tools => 'upload_local_cover_images'} -]; - my %cookies = CGI::Cookie->fetch; my $sid = $cookies{'CGISESSID'}->value; - -my $auth_failure = 1; my ( $auth_status, $sessionID ) = check_cookie_auth( $sid ); my $uid = C4::Auth::get_session($sid)->param('id'); -foreach my $flag_required ( @{$flags_required} ) { - if ( my $flags = haspermission( $uid, $flag_required ) ) { - $auth_failure = 0 if $auth_status eq 'ok'; - } -} +my $allowed = Koha::Schema::Result::UploadedFile->allows_add_by( $uid ); -if ($auth_failure) { +if( $auth_status ne 'ok' || !$allowed ) { send_reply( 'denied' ); exit 0; } -- 1.7.10.4