diff --git a/Koha/CookieManager.pm b/Koha/CookieManager.pm
index 3b655fa12c..2c5ae33516 100644
--- a/Koha/CookieManager.pm
+++ b/Koha/CookieManager.pm
@@ -25,7 +25,28 @@ use CGI::Cookie;
use C4::Context;
-use constant DENY_LIST_VAR => 'do_not_remove_cookie';
+# The cookies on the following list are removed unless koha-conf.xml indicates otherwise.
+# koha-conf.xml may also contain additional cookies to be removed.
+# Not including here: always_show_holds, catalogue_editor_, issues-table-load-immediately-circulation,
+# and ItemEditorSessionTemplateId.
+# TODO Not sure about branch (rotatingcollections), patronSessionConfirmation (circulation.pl)
+use constant MANAGED_COOKIE_PREFIXES => qw{
+ bib_list
+ CGISESSID
+ holdfor holdforclub
+ intranet_bib_list
+ JWT
+ KohaOpacLanguage
+ LastCreatedItem
+ marctagstructure_selectdisplay
+ search_path_code
+ searchToOrder
+};
+use constant PATH_EXCEPTIONS => {
+ always_show_holds => '/cgi-bin/koha/reserve',
+};
+use constant KEEP_COOKIE_CONF_VAR => 'do_not_remove_cookie';
+use constant REMOVE_COOKIE_CONF_VAR => 'remove_cookie';
our $cookies;
@@ -41,18 +62,22 @@ Koha::CookieManager - Object for unified handling of cookies in Koha
# Replace cookies
$cookie_list = $mgr->replace_in_list( [ $cookie1, $cookie2_old ], $cookie2_new );
- # Clear cookies (governed by deny list entries in koha-conf)
+ # Clear cookies
$cookie_list = $mgr->clear_unless( $cookie1, $cookie2, $cookie3_name );
=head1 DESCRIPTION
-The current object allows you to clear cookies in a list based on the deny list
-in koha-conf.xml. It also offers a method to replace the old version of a cookie
+The current object allows you to remove cookies on the hardcoded list
+in this module, refined by 'keep' or 'remove' entries in koha-conf.xml.
+Note that a keep entry overrules a remove.
+
+This module also offers a method to replace the old version of a cookie
by a new one.
-It could be extended by (gradually) routing cookie creation through it in order
-to consistently fill cookie parameters like httponly, secure and samesite flag,
-etc. And could serve to register all our cookies in a central location.
+The module could be extended by (gradually) routing cookie creation
+through it in order to consistently fill cookie parameters like httponly,
+secure and samesite flag, etc. And could serve to register all our cookies
+in a central location.
=head1 METHODS
@@ -64,11 +89,15 @@ etc. And could serve to register all our cookies in a central location.
sub new {
my ( $class, $params ) = @_;
- my $self = bless $params // {}, $class;
- my $denied = C4::Context->config(DENY_LIST_VAR) || []; # expecting scalar or arrayref
- $denied = [$denied] if ref($denied) eq q{};
- $self->{_remove_unless} = { map { $_ => 1 } @$denied };
- $self->{_secure} = C4::Context->https_enabled;
+ my $self = bless $params // {}, $class;
+
+ # Get keep and remove list from koha-conf (scalar or arrayref)
+ my $keep_list = C4::Context->config(KEEP_COOKIE_CONF_VAR) || [];
+ $self->{_keep_list} = ref($keep_list) ? $keep_list : [$keep_list];
+ my $remove_list = C4::Context->config(REMOVE_COOKIE_CONF_VAR) || [];
+ $self->{_remove_list} = ref($remove_list) ? $remove_list : [$remove_list];
+
+ $self->{_secure} = C4::Context->https_enabled;
return $self;
}
@@ -80,9 +109,9 @@ sub new {
Note: in the example above $query->cookie is a list of cookie names as returned
by the CGI object.
- Returns an arrayref of cookie objects: empty, expired cookies for those passed
- by name or objects that are not on the deny list, together with the remaining
- (untouched) cookie objects that are on the deny list.
+ Returns an arrayref of cookie objects: empty, expired cookies for
+ cookies on the remove list, together with the remaining (untouched)
+ cookie objects.
=cut
@@ -102,17 +131,15 @@ sub clear_unless {
}
next if !$name;
- if ( $self->_should_be_cleared($name) ) {
+ if ( $self->_should_be_removed($name) ) {
next if $seen->{$name};
- push @rv, CGI::Cookie->new(
-
- # -expires explicitly omitted to create shortlived 'session' cookie
- # -HttpOnly explicitly set to 0: not really needed here for the
- # cleared httponly cookies, while the js cookies should be 0
- -name => $name, -value => q{}, -HttpOnly => 0,
- $self->{_secure} ? ( -secure => 1 ) : (),
- );
$seen->{$name} = 1; # prevent duplicates
+ if ($type) {
+ $c->max_age(0);
+ push @rv, _correct_path($c);
+ } else {
+ push @rv, _correct_path( CGI::Cookie->new( -name => $name, -value => q{}, '-max-age' => 0 ) );
+ }
} elsif ( $type eq 'CGI::Cookie' ) { # keep the last occurrence
@rv = @{ $self->replace_in_list( \@rv, $c ) };
}
@@ -120,22 +147,6 @@ sub clear_unless {
return \@rv;
}
-sub _should_be_cleared { # when it is not on the deny list in koha-conf
- my ( $self, $name ) = @_;
-
- return if $self->{_remove_unless}->{$name}; # exact match
-
- # Now try the entries as regex
- foreach my $k ( keys %{ $self->{_remove_unless} } ) {
- my $reg = $self->{_remove_unless}->{$k};
-
- # The entry in koha-conf should match the complete string
- # So adding a ^ and $
- return if $name =~ qr/^${k}$/;
- }
- return 1;
-}
-
=head2 replace_in_list
$list2 = $mgr->replace_in_list( $list1, $cookie );
@@ -162,7 +173,44 @@ sub replace_in_list {
return \@result;
}
-=head1 INTERNAL ROUTINES
+# INTERNAL ROUTINES
+
+sub _should_be_removed {
+ my ( $self, $name ) = @_;
+
+ # Is this a controlled cookie? Or is it added as 'keep' or 'remove' in koha-conf.xml?
+ # The conf entries are treated as prefix (no longer as regex).
+ return unless grep { $name =~ /^$_/ } MANAGED_COOKIE_PREFIXES(), @{ $self->{_remove_list} };
+ return !grep { $name =~ /^$_/ } @{ $self->{_keep_list} };
+}
+
+sub _correct_path {
+ my $cookie_object = shift;
+ my $path = PATH_EXCEPTIONS->{ $cookie_object->name } or return $cookie_object;
+ $cookie_object->path($path);
+ return $cookie_object;
+}
+
+=head1 ADDITIONAL COMMENTS
+
+ How do the keep or remove lines in koha-conf.xml work?
+
+ some_cookie
+ The name some_cookie should refer here to a cookie that is on the
+ hardcoded list in this module. If you do not want it to be cleared
+ (removed) on logout, include this line.
+ You might want to do this e.g. for KohaOpacLanguage.
+
+ another_cookie
+ The name another_cookie refers here to a cookie that is not on the
+ hardcoded list but you want this cookie to be cleared/removed on logout.
+ It could be a custom cookie.
+
+ Note that both directives use the cookie name as a prefix. So if you
+ add a remove line for cookie1, it also affects cookie12, etc.
+ Since a keep line overrules a remove line, this allows you to add
+ lines for removing cookie1 and not removing cookie12 in order to
+ remove cookie1, cookie11, cookie13 but not cookie12, etc.
=cut
diff --git a/t/CookieManager.t b/t/CookieManager.t
index b4d3604bff..639b681d12 100755
--- a/t/CookieManager.t
+++ b/t/CookieManager.t
@@ -19,9 +19,10 @@
use Modern::Perl;
use CGI;
-use Data::Dumper qw(Dumper);
+
+#use Data::Dumper qw(Dumper);
use Test::NoWarnings;
-use Test::More tests => 4;
+use Test::More tests => 5;
use t::lib::Mocks;
@@ -29,29 +30,32 @@ use C4::Context;
use Koha::CookieManager;
subtest 'new' => sub {
- plan tests => 3;
+ plan tests => 4;
- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, 'just_one' );
+ t::lib::Mocks::mock_config( Koha::CookieManager::KEEP_COOKIE_CONF_VAR, 'just_one' );
my $cmgr = Koha::CookieManager->new;
- is( scalar keys %{ $cmgr->{_remove_unless} }, 1, 'one entry' );
- is( exists $cmgr->{_secure}, 1, 'secure key found' );
+ is( scalar @{ $cmgr->{_keep_list} }, 1, 'one entry to keep' );
+ is( exists $cmgr->{_secure}, 1, 'secure key found' );
- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, [ 'two', 'entries' ] );
+ t::lib::Mocks::mock_config( Koha::CookieManager::KEEP_COOKIE_CONF_VAR, [ 'two', 'entries' ] );
+ t::lib::Mocks::mock_config( Koha::CookieManager::REMOVE_COOKIE_CONF_VAR, ['test'] );
$cmgr = Koha::CookieManager->new;
- is( scalar keys %{ $cmgr->{_remove_unless} }, 2, 'two entries' );
+ is( scalar @{ $cmgr->{_keep_list} }, 2, 'two entries to keep' );
+ is( scalar @{ $cmgr->{_remove_list} }, 1, 'one entry to remove' );
};
subtest 'clear_unless' => sub {
- plan tests => 17;
+ plan tests => 14;
- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, [ 'aap', 'noot' ] );
+ t::lib::Mocks::mock_config( Koha::CookieManager::KEEP_COOKIE_CONF_VAR, [ 'aap', 'noot' ] );
+ t::lib::Mocks::mock_config( Koha::CookieManager::REMOVE_COOKIE_CONF_VAR, ['mies'] );
my $q = CGI->new;
my $cmgr = Koha::CookieManager->new;
my $cookie1 = $q->cookie( -name => 'aap', -value => 'aap', -expires => '+1d' );
my $cookie2 = $q->cookie( -name => 'noot', -value => 'noot' );
- my $cookie3 = $q->cookie( -name => 'wim', -value => q{wim}, -HttpOnly => 1 );
+ my $cookie3 = $q->cookie( -name => 'wim', -value => q{wim}, -HttpOnly => 0 );
my $cookie4 = $q->cookie( -name => 'aap', -value => q{aap2}, -HttpOnly => 1 );
my $list = [ $cookie1, $cookie2, $cookie3, $cookie4, 'mies', 'zus' ]; # 4 cookies, 2 names
@@ -59,54 +63,47 @@ subtest 'clear_unless' => sub {
is( @{ $cmgr->clear_unless }, 0, 'Empty list' );
is( @{ $cmgr->clear_unless( { hash => 1 }, ['array'], $q ) }, 0, 'Empty list for invalid arguments' );
- # Pass list, expect 5 cookies (3 cleared, last aap kept)
+ # Pass list, expecting 4 cookies (2 kept, 1 untouched, 1 cleared); duplicate aap and zus discarded
my @rv = @{ $cmgr->clear_unless(@$list) };
- is( @rv, 5, '5 expected' );
+ is( @rv, 4, '4 expected' );
is( $rv[0]->name, 'noot', '1st cookie' );
is( $rv[1]->name, 'wim', '2nd cookie' );
is( $rv[2]->name, 'aap', '3rd cookie' );
is( $rv[3]->name, 'mies', '4th cookie' );
- is( $rv[4]->name, 'zus', '5th cookie' );
- is( $rv[0]->value, q{noot}, 'noot not empty' );
- is( $rv[1]->value, q{}, 'wim empty' );
- is( $rv[2]->value, q{aap2}, 'aap not empty' );
- is( $rv[3]->value, q{}, 'mies empty' );
- is( $rv[4]->value, q{}, 'zus empty' );
- is( $rv[1]->httponly, 0, 'cleared wim is not httponly' );
+ is( $rv[0]->value, q{noot}, 'noot kept' );
+ is( $rv[1]->value, q{wim}, 'wim untouched' );
+ is( $rv[2]->value, q{aap2}, 'aap kept, last entry' );
+ is( $rv[3]->value, q{}, 'mies cleared' );
+ is( $rv[1]->httponly, undef, 'wim still not httponly' );
is( $rv[2]->httponly, 1, 'aap httponly' );
- # Test with numeric suffix (via regex)
- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, ['catalogue_editor_\d+'] );
+ # Test with prefix (note trailing underscore)
+ t::lib::Mocks::mock_config( Koha::CookieManager::KEEP_COOKIE_CONF_VAR, 'catalogue_editor_' );
+ t::lib::Mocks::mock_config( Koha::CookieManager::REMOVE_COOKIE_CONF_VAR, 'catalogue_editor' );
$cmgr = Koha::CookieManager->new;
- $cookie1 = $q->cookie( -name => 'catalogue_editor_abc', -value => '1', -expires => '+1y' );
- $cookie2 = $q->cookie( -name => 'catalogue_editor_345', -value => '1', -expires => '+1y' );
- $cookie3 = $q->cookie( -name => 'catalogue_editor_', -value => '1', -expires => '+1y' );
- $cookie4 = $q->cookie( -name => 'catalogue_editor_123x', -value => '1', -expires => '+1y' );
+ $cookie1 = $q->cookie( -name => 'catalogue_editor', -value => '1' );
+ $cookie2 = $q->cookie( -name => 'catalogue_editor2', -value => '2' );
+ $cookie3 = $q->cookie( -name => 'catalogue_editor_3', -value => '3' );
- $list = [ $cookie1, $cookie2, $cookie3, $cookie4 ];
- @rv = @{ $cmgr->clear_unless(@$list) };
- is_deeply(
- [ map { $_->value ? $_->name : () } @rv ],
- ['catalogue_editor_345'],
- 'Cookie2 should be found only'
- );
+ $list = [ $cookie1, $cookie2, $cookie3, 'catalogue_editor4' ];
+ my $result = [ map { defined( $_->max_age ) ? () : $_->name } @{ $cmgr->clear_unless(@$list) } ];
+ is_deeply( $result, ['catalogue_editor_3'], 'Only cookie3 is kept (not expired)' );
+};
- # Test with another regex (yes, highly realistic examples :)
- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, ['next_\w+_number\d{2}_(now|never)'] );
- $cmgr = Koha::CookieManager->new;
- my $cookie5;
- $cookie1 = $q->cookie( -name => 'next_mynewword_number99_never', -value => '1', -expires => '+1y' ); #fine
- $cookie2 = $q->cookie( -name => 'prefixed_next_mynewword_number99_never', -value => '1', -expires => '+1y' )
- ; # wrong prefix
- $cookie3 = $q->cookie( -name => 'next_mynew-word_number99_never', -value => '1', -expires => '+1y' )
- ; # wrong: hyphen in word
- $cookie4 =
- $q->cookie( -name => 'mynewword_number999_never', -value => '1', -expires => '+1y' ); # wrong: three digits
- $cookie5 =
- $q->cookie( -name => 'next_mynewword_number99_always', -value => '1', -expires => '+1y' ); # wrong: always
- @rv = @{ $cmgr->clear_unless( $cookie1, $cookie2, $cookie3, $cookie4, $cookie5 ) };
- is_deeply( [ map { $_->value ? $_->name : () } @rv ], ['next_mynewword_number99_never'], 'Only cookie1 matched' );
+subtest 'path exception' => sub {
+ plan tests => 4;
+ t::lib::Mocks::mock_config( Koha::CookieManager::REMOVE_COOKIE_CONF_VAR, ['always_show_holds'] );
+ my $q = CGI->new;
+ my $cmgr = Koha::CookieManager->new;
+ my $cookie1 = $q->cookie( -name => 'always_show_holds', -value => 'DO', path => '/cgi-bin/koha/reserve' );
+ my @rv = @{ $cmgr->clear_unless($cookie1) };
+ is( $rv[0]->name, 'always_show_holds', 'Check name' );
+ is( $rv[0]->path, '/cgi-bin/koha/reserve', 'Check path' );
+ is( $rv[0]->max_age, 0, 'Check max_age' );
+ my $cookie2 = $q->cookie( -name => 'always_show_holds', -value => 'DONT' ); # default path
+ @rv = @{ $cmgr->clear_unless($cookie2) };
+ is( $rv[0]->path, '/cgi-bin/koha/reserve', 'Check path cookie2, corrected here' );
};
subtest 'replace_in_list' => sub {