From 4f7803e469a41cafb993c6d41e6481499e6cca93 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 16 Apr 2014 16:50:23 +0200 Subject: [PATCH] Bug 12098: Fix C4::Serials::can_edit_subscription This patch fixes a problem whereby staff users could edit subscriptions they are not permitted to by going directly to the subscription details page. It also adds some unit tests for the can_edit_subscription routine and add a new can_show_subscription routines. Signed-off-by: Kyle M Hall Signed-off-by: Katrin Fischer Notes on second patch. Signed-off-by: Galen Charlton --- C4/Serials.pm | 71 ++++++++++-- t/db_dependent/Serials_2.t | 223 +++++++++++++++++++++++++++++++++---- 2 files changed, 259 insertions(+), 35 deletions(-) diff --git a/C4/Serials.pm b/C4/Serials.pm index 76b3f0bc39..892996fa26 100644 --- a/C4/Serials.pm +++ b/C4/Serials.pm @@ -2849,18 +2849,65 @@ sub can_edit_subscription { return 0 unless C4::Context->userenv; my $flags = C4::Context->userenv->{flags}; $userid ||= C4::Context->userenv->{'id'}; - my $independent_branches = C4::Context->preference('IndependentBranches'); - return 1 unless $independent_branches; - if( C4::Context->IsSuperLibrarian() - or C4::Auth::haspermission( $userid, {serials => 'superserials'}), - or C4::Auth::haspermission( $userid, {serials => 'edit_subscription'}), - or not defined $subscription->{branchcode} - or $subscription->{branchcode} eq '' - or $subscription->{branchcode} eq C4::Context->userenv->{'branch'} - ) { - return 1; - } - return 0; + + if ( C4::Context->preference('IndependentBranches') ) { + return 1 + if C4::Context->IsSuperLibrarian() + or + C4::Auth::haspermission( $userid, { serials => 'superserials' } ) + or ( + C4::Auth::haspermission( $userid, + { serials => 'edit_subscription' } ) + and ( not defined $subscription->{branchcode} + or $subscription->{branchcode} eq '' + or $subscription->{branchcode} eq + C4::Context->userenv->{'branch'} ) + ); + } + else { + return 1 + if C4::Context->IsSuperLibrarian() + or + C4::Auth::haspermission( $userid, { serials => 'superserials' } ) + or C4::Auth::haspermission( + $userid, { serials => 'edit_subscription' } + ), + ; + } + return 0; +} + +sub can_show_subscription { + my ( $subscription, $userid ) = @_; + return 0 unless C4::Context->userenv; + my $flags = C4::Context->userenv->{flags}; + $userid ||= C4::Context->userenv->{'id'}; + + if ( C4::Context->preference('IndependentBranches') ) { + return 1 + if C4::Context->IsSuperLibrarian() + or + C4::Auth::haspermission( $userid, { serials => 'superserials' } ) + or ( + C4::Auth::haspermission( $userid, + { serials => '*' } ) + and ( not defined $subscription->{branchcode} + or $subscription->{branchcode} eq '' + or $subscription->{branchcode} eq + C4::Context->userenv->{'branch'} ) + ); + } + else { + return 1 + if C4::Context->IsSuperLibrarian() + or + C4::Auth::haspermission( $userid, { serials => 'superserials' } ) + or C4::Auth::haspermission( + $userid, { serials => '*' } + ), + ; + } + return 0; } 1; diff --git a/t/db_dependent/Serials_2.t b/t/db_dependent/Serials_2.t index 3d921b8d18..d1dff21306 100644 --- a/t/db_dependent/Serials_2.t +++ b/t/db_dependent/Serials_2.t @@ -1,15 +1,21 @@ #!/usr/bin/perl use Modern::Perl; -use Test::More tests => 5; +use Test::More;# tests => 5; #FIXME uncomment me use MARC::Record; use C4::Biblio qw( AddBiblio ); -use C4::Context; +use C4::Members qw( AddMember ); +use t::lib::Mocks; use_ok('C4::Serials'); use_ok('C4::Budgets'); +# Mock userenv +local $SIG{__WARN__} = sub { warn $_[0] unless $_[0] =~ /redefined/ }; +my $userenv; +*C4::Context::userenv = \&Mock_userenv; + my $dbh = C4::Context->dbh; $dbh->{AutoCommit} = 0; $dbh->{RaiseError} = 1; @@ -24,10 +30,12 @@ $record->append_fields( ); my ( $biblionumber, $biblioitemnumber ) = C4::Biblio::AddBiblio($record, ''); +my $my_branch = 'CPL'; +my $another_branch = 'MPL'; my $budgetid; my $bpid = AddBudgetPeriod({ - budget_period_startdate => '01-01-2015', - budget_period_enddate => '12-31-2015', + budget_period_startdate => '2015-01-01', + budget_period_enddate => '2015-12-31', budget_description => "budget desc" }); @@ -41,38 +49,207 @@ my $budget_id = AddBudget({ budget_period_id => $bpid }); -my $subscriptionid = NewSubscription( - undef, "", undef, undef, $budget_id, $biblionumber, +my $subscriptionid_from_my_branch = NewSubscription( + undef, $my_branch, undef, undef, $budget_id, $biblionumber, + '2013-01-01', undef, undef, undef, undef, + undef, undef, undef, undef, undef, undef, + 1, "notes",undef, '2013-01-01', undef, undef, + undef, undef, 0, "intnotes", 0, + undef, undef, 0, undef, '2013-12-31', 0 +); +die unless $subscriptionid_from_my_branch; + +my $subscriptionid_from_another_branch = NewSubscription( + undef, $another_branch, undef, undef, $budget_id, $biblionumber, '2013-01-01', undef, undef, undef, undef, undef, undef, undef, undef, undef, undef, 1, "notes",undef, '2013-01-01', undef, undef, undef, undef, 0, "intnotes", 0, undef, undef, 0, undef, '2013-12-31', 0 ); -die unless $subscriptionid; -my $subscription = GetSubscription( $subscriptionid ); -is( C4::Serials::can_edit_subscription($subscription), 0, "cannot edit a subscription without userenv set"); +my $subscription_from_my_branch = GetSubscription( $subscriptionid_from_my_branch ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 0, "cannot edit a subscription without userenv set"); -my @USERENV = ( - 1, - 'test', - 'MASTERTEST', - 'Test', - 'Test', - 't', - 0, - 0, +my $userid = 'my_userid'; +my $borrowernumber = C4::Members::AddMember( + firstname => 'my fistname', + surname => 'my surname', + categorycode => 'S', + branchcode => $my_branch, + userid => $userid, ); -C4::Context->_new_userenv ('DUMMY_SESSION_ID'); -C4::Context->set_userenv ( @USERENV ); +$userenv = { flags => 1, id => $borrowernumber, branch => '' }; # FIXME Not sure about this test # Can edit a subscription -my $userenv = C4::Context->userenv; -is( C4::Serials::can_edit_subscription($subscription), 1, "User can edit a subscription with an empty branchcode"); -#TODO add UT when C4::Auth->set_permissions (or setuserflags) will exist. +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 1, "User can edit a subscription with an empty branchcode"); + +my $subscription_from_another_branch = GetSubscription( $subscriptionid_from_another_branch ); + +$userenv->{id} = $userid; +$userenv->{branch} = $my_branch; + +# Branches are independent +t::lib::Mocks::mock_preference( "IndependentBranches", 1 ); +set_flags( 'superlibrarian', $borrowernumber ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 1, +"With IndependentBranches, superlibrarian can edit a subscription from his branch" +); +is( C4::Serials::can_edit_subscription($subscription_from_another_branch), 1, +"With IndependentBranches, superlibrarian can edit a subscription from another branch" +); +is( C4::Serials::can_show_subscription($subscription_from_my_branch), 1, +"With IndependentBranches, superlibrarian can show a subscription from his branch" +); +is( C4::Serials::can_show_subscription($subscription_from_another_branch), 1, +"With IndependentBranches, superlibrarian can show a subscription from another branch" +); + +set_flags( 'superserials', $borrowernumber ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 1, +"With IndependentBranches, superserials can edit a subscription from his branch" +); +is( C4::Serials::can_edit_subscription($subscription_from_another_branch), 1, +"With IndependentBranches, superserials can edit a subscription from another branch" +); +is( C4::Serials::can_show_subscription($subscription_from_my_branch), 1, +"With IndependentBranches, superserials can show a subscription from his branch" +); +is( C4::Serials::can_show_subscription($subscription_from_another_branch), 1, +"With IndependentBranches, superserials can show a subscription from another branch" +); + + +set_flags( 'edit_subscription', $borrowernumber ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 1, +"With IndependentBranches, edit_subscription can edit a subscription from his branch" +); +is( C4::Serials::can_edit_subscription($subscription_from_another_branch), 0, +"With IndependentBranches, edit_subscription cannot edit a subscription from another branch" +); +is( C4::Serials::can_show_subscription($subscription_from_my_branch), 1, +"With IndependentBranches, show_subscription can show a subscription from his branch" +); +is( C4::Serials::can_show_subscription($subscription_from_another_branch), 0, +"With IndependentBranches, show_subscription cannot show a subscription from another branch" +); + +set_flags( 'renew_subscription', $borrowernumber ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 0, +"With IndependentBranches, renew_subscription cannot edit a subscription from his branch" +); +is( C4::Serials::can_edit_subscription($subscription_from_another_branch), 0, +"With IndependentBranches, renew_subscription cannot edit a subscription from another branch" +); +is( C4::Serials::can_show_subscription($subscription_from_my_branch), 1, +"With IndependentBranches, renew_subscription can show a subscription from his branch" +); +is( C4::Serials::can_show_subscription($subscription_from_another_branch), 0, +"With IndependentBranches, renew_subscription cannot show a subscription from another branch" +); + + +# Branches are not independent +t::lib::Mocks::mock_preference( "IndependentBranches", 0 ); +set_flags( 'superlibrarian', $borrowernumber ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 1, +"Without IndependentBranches, superlibrarian can edit a subscription from his branch" +); +is( C4::Serials::can_edit_subscription($subscription_from_another_branch), 1, +"Without IndependentBranches, superlibrarian can edit a subscription from another branch" +); +is( C4::Serials::can_show_subscription($subscription_from_my_branch), 1, +"Without IndependentBranches, superlibrarian can show a subscription from his branch" +); +is( C4::Serials::can_show_subscription($subscription_from_another_branch), 1, +"Without IndependentBranches, superlibrarian can show a subscription from another branch" +); + +set_flags( 'superserials', $borrowernumber ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 1, +"Without IndependentBranches, superserials can edit a subscription from his branch" +); +is( C4::Serials::can_edit_subscription($subscription_from_another_branch), 1, +"Without IndependentBranches, superserials can edit a subscription from another branch" +); +is( C4::Serials::can_show_subscription($subscription_from_my_branch), 1, +"Without IndependentBranches, superserials can show a subscription from his branch" +); +is( C4::Serials::can_show_subscription($subscription_from_another_branch), 1, +"Without IndependentBranches, superserials can show a subscription from another branch" +); + +set_flags( 'edit_subscription', $borrowernumber ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 1, +"Without IndependentBranches, edit_subscription can edit a subscription from his branch" +); +is( C4::Serials::can_edit_subscription($subscription_from_another_branch), 1, +"Without IndependentBranches, edit_subscription can edit a subscription from another branch" +); +is( C4::Serials::can_show_subscription($subscription_from_my_branch), 1, +"Without IndependentBranches, show_subscription can show a subscription from his branch" +); +is( C4::Serials::can_show_subscription($subscription_from_another_branch), 1, +"Without IndependentBranches, show_subscription can show a subscription from another branch" +); + +set_flags( 'renew_subscription', $borrowernumber ); +is( C4::Serials::can_edit_subscription($subscription_from_my_branch), 0, +"Without IndependentBranches, renew_subscription cannot edit a subscription from his branch" +); +is( C4::Serials::can_edit_subscription($subscription_from_another_branch), 0, +"Without IndependentBranches, renew_subscription cannot edit a subscription from another branch" +); +is( C4::Serials::can_show_subscription($subscription_from_my_branch), 1, +"Without IndependentBranches, renew_subscription cannot show a subscription from his branch" +); +is( C4::Serials::can_show_subscription($subscription_from_another_branch), 1, +"Without IndependentBranches, renew_subscription cannot show a subscription from another branch" +); + + $dbh->rollback; + +done_testing; + +# C4::Context->userenv +sub Mock_userenv { + return $userenv; +} + +sub set_flags { + my ( $flags, $borrowernumber ) = @_; + my $superlibrarian_flags = 1; + if ( $flags eq 'superlibrarian' ) { + $dbh->do( + q| + UPDATE borrowers SET flags=? WHERE borrowernumber=? + |, {}, $superlibrarian_flags, $borrowernumber + ); + $userenv->{flags} = $superlibrarian_flags; + } + else { + $dbh->do( + q| + UPDATE borrowers SET flags=? WHERE borrowernumber=? + |, {}, 0, $borrowernumber + ); + $userenv->{flags} = 0; + my ( $module_bit, $code ) = ( '15', $flags ); + $dbh->do( + q| + DELETE FROM user_permissions where borrowernumber=? + |, {}, $borrowernumber + ); + + $dbh->do( + q| + INSERT INTO user_permissions( borrowernumber, module_bit, code ) VALUES ( ?, ?, ? ) + |, {}, $borrowernumber, $module_bit, $code + ); + } +} -- 2.39.5