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 <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Notes on second patch.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

diff --git a/C4/Serials.pm b/C4/Serials.pm
index 76b3f0b..892996f 100644 (file)
--- 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 3d921b8..d1dff21 100644 (file)
--- 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
+        );
+    }
+}