Bug 29957: (follow-up) Turn allow list into deny list

See the discussion on the Bugzilla report.
It might be easier to work with a deny list.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Koha/CookieManager.pm

@@ -24,7 +24,7 @@ use CGI::Cookie;
 
 use C4::Context;
 
-use constant ALLOW_LIST_VAR => 'removable_cookie';
+use constant DENY_LIST_VAR => 'do_not_remove_cookie';
 
 our $cookies;
 
@@ -40,15 +40,14 @@ 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 koha-conf removable_cookie lines)
-    $cookie_list = $mgr->clear_if_allowed( $cookie1, $cookie2, $cookie3_name );
-
+    # Clear cookies (governed by deny list entries in koha-conf)
+    $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 allow
-list in koha-conf.xml. It also offers a method to replace the old version of
-a cookie by a new one.
+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
+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,
@@ -65,28 +64,28 @@ etc. And could serve to register all our cookies in a central location.
 sub new {
     my ( $class, $params ) = @_;
     my $self = bless $params//{}, $class;
-    my $allowed = C4::Context->config(ALLOW_LIST_VAR) || []; # expecting scalar or arrayref
-    $allowed = [ $allowed ] if ref($allowed) eq q{};
-    $self->{_remove_allowed} = { map { $_ => 1 } @$allowed };
+    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;
     return $self;
 }
 
-=head2 clear_if_allowed
+=head2 clear_unless
 
-    $cookies = $self->clear_if_allowed( $query->cookie, @$cookies );
+    $cookies = $self->clear_unless( $query->cookie, @$cookies );
 
     Arguments: either cookie names or cookie objects (CGI::Cookie).
     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 object that are on the allow list, together with the remaining
-    (untouched) cookie objects not on that list.
+    by name or objects that are not on the deny list, together with the remaining
+    (untouched) cookie objects that are on the deny list.
 
 =cut
 
-sub clear_if_allowed {
+sub clear_unless {
     my ( $self, @cookies ) = @_;
     my @rv;
     my $seen = {};
@@ -101,8 +100,8 @@ sub clear_if_allowed {
             $name = $c;
         }
 
-        if( $self->{_remove_allowed}->{$name} ) {
-            next if $seen->{ $name };
+        if( !$self->{_remove_unless}->{$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
@@ -110,9 +109,9 @@ sub clear_if_allowed {
                 -name => $name, -value => q{}, -HttpOnly => 0,
                 $self->{_secure} ? ( -secure => 1 ) : (),
             );
-            $seen->{ $name } = 1; # prevent duplicates
-        } elsif( $type eq 'CGI::Cookie' ) {
-            push @rv, $c;
+            $seen->{$name} = 1; # prevent duplicates
+        } elsif( $type eq 'CGI::Cookie' ) { # keep the last occurrence
+            @rv = @{ $self->replace_in_list( \@rv, $c ) };
         }
     }
     return \@rv;

t/CookieManager.t

@@ -30,59 +30,49 @@ use Koha::CookieManager;
 subtest 'new' => sub {
     plan tests => 3;
 
-    t::lib::Mocks::mock_config( Koha::CookieManager::ALLOW_LIST_VAR, 'just_one' );
+    t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, 'just_one' );
     my $cmgr = Koha::CookieManager->new;
-    is( scalar keys %{$cmgr->{_remove_allowed}}, 1, 'one entry' );
+    is( scalar keys %{$cmgr->{_remove_unless}}, 1, 'one entry' );
     is( exists $cmgr->{_secure}, 1, 'secure key found' );
 
-    t::lib::Mocks::mock_config( Koha::CookieManager::ALLOW_LIST_VAR, [ 'two', 'entries' ] );
+    t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, [ 'two', 'entries' ] );
     $cmgr = Koha::CookieManager->new;
-    is( scalar keys %{$cmgr->{_remove_allowed}}, 2, 'two entries' );
+    is( scalar keys %{$cmgr->{_remove_unless}}, 2, 'two entries' );
 };
 
-subtest 'clear_if_allowed' => sub {
-    plan tests => 13;
+subtest 'clear_unless' => sub {
+    plan tests => 15;
 
-    t::lib::Mocks::mock_config( Koha::CookieManager::ALLOW_LIST_VAR, [ 'aap', 'noot', 'mies' ] );
+    t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, [ 'aap', 'noot' ] );
 
     my $q = CGI->new;
     my $cmgr = Koha::CookieManager->new;
 
-    my $cookie1 = $q->cookie(
-        -name => 'aap',
-        -value => 'aap',
-        -expires => '+1d',
-        -HttpOnly => 1,
-        -secure => 1,
-    );
-    my $cookie2 = $q->cookie(
-        -name => 'noot',
-        -value => 'noot',
-        -expires => '+1d',
-        -HttpOnly => 1,
-        -secure => 1,
-    );
+    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 $cookie4 = $q->cookie( -name => 'aap', -value => q{aap2} );
+    my $cookie4 = $q->cookie( -name => 'aap', -value => q{aap2}, -HttpOnly => 1 );
     my $list = [ $cookie1, $cookie2, $cookie3, $cookie4, 'mies', 'zus' ]; # 4 cookies, 2 names
 
     # No results expected
-    is( @{$cmgr->clear_if_allowed}, 0, 'Empty list' );
-    is( @{$cmgr->clear_if_allowed( 'scalar', [], $q )}, 0, 'Empty list for invalid arguments' );
+    is( @{$cmgr->clear_unless}, 0, 'Empty list' );
+    is( @{$cmgr->clear_unless( { hash => 1 }, [ 'array' ], $q )}, 0, 'Empty list for invalid arguments' );
 
-    # Pass list, expect 4 cookies (3 cleared)
-    my @rv = @{$cmgr->clear_if_allowed( @$list )};
-    is( @rv, 4, 'Four expected' );
-    is( $rv[0]->name, 'aap', 'First cookie' );
-    is( $rv[1]->name, 'noot', '2nd cookie' );
-    is( $rv[2]->name, 'wim', '3rd cookie' );
+    # Pass list, expect 5 cookies (3 cleared, last aap kept)
+    my @rv = @{$cmgr->clear_unless( @$list )};
+    is( @rv, 5, '5 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[0]->value, q{}, 'aap should be empty' );
-    is( $rv[1]->value, q{}, 'noot should be empty' );
-    is( $rv[2]->value, 'wim', 'wim not empty' );
+    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[0]->httponly, 0, 'cleared aap isnt httponly' );
-    is( $rv[2]->httponly, 1, 'wim still httponly' );
+    is( $rv[4]->value, q{}, 'zus empty' );
+    is( $rv[1]->httponly, 0, 'cleared wim isnt httponly' );
+    is( $rv[2]->httponly, 1, 'aap httponly' );
 };
 
 subtest 'replace_in_list' => sub {