Bugzilla – Attachment 132047 Details for
Bug 29957
Cookies not removed after logout
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 29957: (follow-up) Turn allow list into deny list
Bug-29957-follow-up-Turn-allow-list-into-deny-list.patch (text/plain), 8.19 KB, created by
Marcel de Rooy
on 2022-03-23 08:12:38 UTC
(
hide
)
Description:
Bug 29957: (follow-up) Turn allow list into deny list
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2022-03-23 08:12:38 UTC
Size:
8.19 KB
patch
obsolete
>From 132f549e30e6f100913af7f6ec1baca690ba5de0 Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Wed, 23 Mar 2022 07:41:54 +0000 >Subject: [PATCH] Bug 29957: (follow-up) Turn allow list into deny list >Content-Type: text/plain; charset=utf-8 > >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> >--- > Koha/CookieManager.pm | 39 +++++++++++++-------------- > t/CookieManager.t | 62 ++++++++++++++++++------------------------- > 2 files changed, 45 insertions(+), 56 deletions(-) > >diff --git a/Koha/CookieManager.pm b/Koha/CookieManager.pm >index cba829a196..d7680d8cc0 100644 >--- a/Koha/CookieManager.pm >+++ b/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; >diff --git a/t/CookieManager.t b/t/CookieManager.t >index c7c46e35af..3c49035451 100755 >--- a/t/CookieManager.t >+++ b/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' ); >- >- # 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' ); >+ 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) >+ 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 { >-- >2.20.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 29957
:
129871
|
129872
|
130005
|
130006
|
131428
|
131750
|
131751
|
131752
|
131807
|
131808
|
131809
|
131810
|
131824
|
131825
|
131826
|
131827
|
131828
|
131840
|
131841
|
131842
|
131843
|
131856
|
132045
|
132046
|
132047
|
132105
|
132106
|
132107
|
132108
|
132109
|
132110
|
132111
|
132115
|
132210
|
132211
|
132231
|
132232
|
132233
|
132234
|
132235
|
132236
|
132237
|
132238
|
132239
|
132288
|
132289
|
132290
|
132291
|
132292
|
132293
|
132294
|
132295
|
132296