do some validation of the KohaOpacRecentSearches cookie

Add validation of the value of the KohaOpacRecentSearches.  In
particular, this patch avoids the generation of an internal server
error when the OPAC is presented with an old cookie that uses the
old Storable-based serialization.

This patch also moves parsing of the cookie value into a
new routine in C4::Auth, ParseSearchHistoryCookie, and adds
a test case.

To test (in conjunction with the previous patch):

Exercise the OPAC search history functionality, after
turning on the EnableOpacSearchHistory syspref:

- As an anonymous user, conduct a variety of searches,
  including ones that include non-ASCII characters
- Check the search history and verify that all searches
  are listed
- Apply this patch and the previous one.
- Do *not* clear the KohaOpacRecentSearches cookie
- Check the search history and verify that no searches
  are listed any more
- As an anonymous user, conduct a variety of searches,
  including ones that include non-ASCII characters
- Check the search history and verify that all searches
  are listed
- Log into the OPAC
- Verify that current and past searches are listed in
  search history.

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

diff --git a/C4/Auth.pm b/C4/Auth.pm
index 9d80ab6..1e17e59 100644 (file)
--- a/C4/Auth.pm
+++ b/C4/Auth.pm
@@ -46,7 +46,9 @@ BEGIN {
     $debug       = $ENV{DEBUG};
     @ISA         = qw(Exporter);
     @EXPORT      = qw(&checkauth &get_template_and_user &haspermission &get_user_subpermissions);
-    @EXPORT_OK   = qw(&check_api_auth &get_session &check_cookie_auth &checkpw &get_all_subpermissions &get_user_subpermissions);
+    @EXPORT_OK   = qw(&check_api_auth &get_session &check_cookie_auth &checkpw &get_all_subpermissions &get_user_subpermissions
+                      ParseSearchHistoryCookie
+                   );
     %EXPORT_TAGS = ( EditPermissions => [qw(get_all_subpermissions get_user_subpermissions)] );
     $ldap        = C4::Context->config('useldapserver') || 0;
     $cas         = C4::Context->preference('casAuthentication');
@@ -251,29 +253,25 @@ sub get_template_and_user {
 
             # And if there's a cookie with searches performed when the user was not logged in,
             # we add them to the logged-in search history
-            my $searchcookie = $in->{'query'}->cookie('KohaOpacRecentSearches');
-            if ($searchcookie){
-                $searchcookie = uri_unescape($searchcookie);
-                    my @recentSearches = @{decode_json($searchcookie) || []};
-                if (@recentSearches) {
-                    my $sth = $dbh->prepare($SEARCH_HISTORY_INSERT_SQL);
-                    $sth->execute( $borrowernumber,
-                               $in->{'query'}->cookie("CGISESSID"),
-                               $_->{'query_desc'},
-                               $_->{'query_cgi'},
-                               $_->{'total'},
-                               $_->{'time'},
-                            ) foreach @recentSearches;
-
-                    # And then, delete the cookie's content
-                    my $newsearchcookie = $in->{'query'}->cookie(
-                                                -name => 'KohaOpacRecentSearches',
-                                                -value => encode_json([]),
-                                                -HttpOnly => 1,
-                                                -expires => ''
-                                             );
-                    $cookie = [$cookie, $newsearchcookie];
-                }
+            my @recentSearches = ParseSearchHistoryCookie($in->{'query'});
+            if (@recentSearches) {
+                my $sth = $dbh->prepare($SEARCH_HISTORY_INSERT_SQL);
+                $sth->execute( $borrowernumber,
+                           $in->{'query'}->cookie("CGISESSID"),
+                           $_->{'query_desc'},
+                           $_->{'query_cgi'},
+                           $_->{'total'},
+                           $_->{'time'},
+                        ) foreach @recentSearches;
+
+                # And then, delete the cookie's content
+                my $newsearchcookie = $in->{'query'}->cookie(
+                                            -name => 'KohaOpacRecentSearches',
+                                            -value => encode_json([]),
+                                            -HttpOnly => 1,
+                                            -expires => ''
+                                         );
+                $cookie = [$cookie, $newsearchcookie];
             }
         }
     }
@@ -290,14 +288,9 @@ sub get_template_and_user {
      # Anonymous opac search history
      # If opac search history is enabled and at least one search has already been performed
      if (C4::Context->preference('EnableOpacSearchHistory')) {
-        my $searchcookie = $in->{'query'}->cookie('KohaOpacRecentSearches');
-        if ($searchcookie){
-            $searchcookie = uri_unescape($searchcookie);
-                my @recentSearches = @{decode_json($searchcookie) || []};
-         # We show the link in opac
-            if (@recentSearches) {
-                $template->param(ShowOpacRecentSearchLink => 1);
-            }
+        my @recentSearches = ParseSearchHistoryCookie($in->{'query'}); 
+        if (@recentSearches) {
+            $template->param(ShowOpacRecentSearchLink => 1);
         }
      }
 
@@ -1722,6 +1715,15 @@ sub getborrowernumber {
     return 0;
 }
 
+sub ParseSearchHistoryCookie {
+    my $input = shift;
+    my $search_cookie = $input->cookie('KohaOpacRecentSearches');
+    return () unless $search_cookie;
+    my $obj = eval { decode_json(uri_unescape($search_cookie)) };
+    return () unless defined $obj;
+    return () unless ref $obj eq 'ARRAY';
+    return @{ $obj };
+}
 
 END { }    # module clean-up code here (global destructor)
 1;

diff --git a/opac/opac-search-history.pl b/opac/opac-search-history.pl
index 5536139..67c620b 100755 (executable)
--- a/opac/opac-search-history.pl
+++ b/opac/opac-search-history.pl
@@ -20,7 +20,7 @@
 use strict;
 use warnings;
 
-use C4::Auth qw(:DEFAULT get_session);
+use C4::Auth qw(:DEFAULT get_session ParseSearchHistoryCookie);
 use CGI;
 use JSON qw/decode_json encode_json/;
 use C4::Context;
@@ -65,10 +65,7 @@ if (!$loggedinuser) {
     # Showing search history
     } else {
 
-       # Getting the cookie
-       my $searchcookie = $cgi->cookie('KohaOpacRecentSearches');
-       if ($searchcookie && decode_json(uri_unescape($searchcookie))) {
-           my @recentSearches = @{decode_json(uri_unescape($searchcookie))};
+        my @recentSearches = ParseSearchHistoryCookie($cgi);
            if (@recentSearches) {
 
                # As the dates are stored as unix timestamps, let's do some formatting
@@ -90,7 +87,6 @@ if (!$loggedinuser) {
 
                $template->param(recentSearches => \@recentSearches);
            }
-       }
     }
 } else {
 # And if the user is logged in, we deal with the database

diff --git a/opac/opac-search.pl b/opac/opac-search.pl
index ced8ae4..7d2b7a6 100755 (executable)
--- a/opac/opac-search.pl
+++ b/opac/opac-search.pl
@@ -42,7 +42,7 @@ for ( $searchengine ) {
 }
 
 use C4::Output;
-use C4::Auth qw(:DEFAULT get_session);
+use C4::Auth qw(:DEFAULT get_session ParseSearchHistoryCookie);
 use C4::Languages qw(getAllLanguages);
 use C4::Search;
 use C4::Biblio;  # GetBiblioData
@@ -619,16 +619,7 @@ for (my $i=0;$i<@servers;$i++) {
         # Opac search history
         my $newsearchcookie;
         if (C4::Context->preference('EnableOpacSearchHistory')) {
-            my @recentSearches;
-
-            # Getting the (maybe) already sent cookie
-            my $searchcookie = $cgi->cookie('KohaOpacRecentSearches');
-            if ($searchcookie){
-                $searchcookie = uri_unescape($searchcookie);
-                if (decode_json($searchcookie)) {
-                    @recentSearches = @{decode_json($searchcookie)};
-                }
-            }
+            my @recentSearches = ParseSearchHistoryCookie($cgi);
 
             # Adding the new search if needed
             my $path_info = $cgi->url(-path_info=>1);

diff --git a/t/Auth_ParseSearchHistoryCookie.t b/t/Auth_ParseSearchHistoryCookie.t
new file mode 100644 (file)
index 0000000..507ac91
--- /dev/null
+++ b/t/Auth_ParseSearchHistoryCookie.t
@@ -0,0 +1,43 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Test::More tests => 3;
+
+use_ok('C4::Auth', qw/ParseSearchHistoryCookie/);
+
+my $valid_cookie = "%5B%7B%22time%22%3A1374978877%2C%22query_cgi%22%3A%22idx%3D%26q%3Dhistory%26branch_group_limit%3D%22%2C%22total%22%3A2%2C%22query_desc%22%3A%22kw%2Cwrdl%3A%20history%2C%20%22%7D%5D";
+my $expected_recent_searches = [
+    {
+        'time' => 1374978877,
+        'query_cgi' => 'idx=&q=history&branch_group_limit=',
+        'total' => 2,
+        'query_desc' => 'kw,wrdl: history, '
+    }
+];
+
+my $input = CookieSimulator->new($valid_cookie);
+my @recent_searches = ParseSearchHistoryCookie($input);
+is_deeply(\@recent_searches, $expected_recent_searches, 'parsed valid search history cookie value');
+
+# simulate bit of a Storable-based search history cookie
+my $invalid_cookie = "%04%08%0812345";
+$input = CookieSimulator->new($invalid_cookie);
+@recent_searches = ParseSearchHistoryCookie($input);
+is_deeply(\@recent_searches, [], 'got back empty search history list if given invalid cookie');
+
+package CookieSimulator;
+
+sub new {
+    my ($class, $str) = @_;
+    my $val = [ $str ];
+    return bless $val, $class;
+}
+
+sub cookie {
+    my $self = shift;
+    return $self->[0];
+}
+
+1;