From 10ed783e3686b8e17e4b78446384def5cae6e24f Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Sat, 11 Sep 2021 11:51:38 +0000 Subject: [PATCH] Bug 28999: Remove (un)load, add reset; apply returns result now Content-Type: text/plain; charset=utf-8 Remove load but add $defaults in new. Replace unload by reset, restoring defaults. Replace warn en verbose by a dump variable in apply. Do not return $blocked anymore but return the filtered input in apply. New check method returns true when no entries were blocked. New keys method to list current allow list entries. --- Koha/AllowList.pm | 86 ++++++++++++++++++++++------------------------ t/Koha/AllowList.t | 69 +++++++++++++++++++++---------------- 2 files changed, 81 insertions(+), 74 deletions(-) diff --git a/Koha/AllowList.pm b/Koha/AllowList.pm index 551d09a401..6145e43060 100644 --- a/Koha/AllowList.pm +++ b/Koha/AllowList.pm @@ -27,19 +27,21 @@ Koha::AllowList - Allow list implementation base class =head3 new - my $allow_list = Koha::AllowList->new; + my $allow_list = Koha::AllowList->new({ defaults => $list }); Constructor. =cut -our $defaults = []; - sub new { - my ($class, $args) = @_; - $args //= {}; - my $self = bless( $args, $class ); + my ( $class, $args ) = @_; # args hash with keys: defaults + $args = {} if !$args or ref($args) ne 'HASH'; + my $defaults = ref($args->{defaults}) eq 'ARRAY' ? $args->{defaults} : []; + + my $self = bless( {}, $class ); + $self->{_defaults} = $defaults; $self->{_entries} = {}; + $self->add( @$defaults ); return $self; } @@ -51,7 +53,6 @@ sub new { sub add { my ( $self, @args ) = @_; - # TODO Could be extended by passing things like column3 => { staff => 1 } foreach my $arg ( @args ) { $self->{_entries}->{$arg} = 1; } @@ -72,16 +73,13 @@ sub remove { =head3 apply - my $blocked = $allowlist->apply({ - input => $hashref | $arrayref, verbose => 1, dry_run => 0, + my $result = $allowlist->apply({ + input => $hashref | $arrayref, dump => $dump, }); - Apply an allowlist to input data. Note that we modify the passed - reference unless the dry_run flag is true. - The verbose flag controls the warn statement. - - Returns a hash of blocked entries. Might be useful for logging purposes. - When you pass an arrayref, the return looks like: { blocked1 => 1, blocked2 => 1, .. } + Apply an allowlist to input data. Returns result as hash- or arrayref. + If you pass a dump hashref, blocked entries will be written. When you + pass an arrayref, the dump looks like: { blocked1 => 1, .. } =cut @@ -89,66 +87,64 @@ sub apply { my ( $self, $params ) = @_; my $input = $params->{input} or return; return if ref($input) ne 'HASH' && ref($input) ne 'ARRAY'; - my $dry = $params->{dry_run}; - my $blocked = {}; + my $dump = {}; + my $result; if( ref($input) eq 'ARRAY' ) { - my ( @ok, @nok ); - map { $self->{_entries}->{$_} ? push @ok, $_ : push @nok, $_; } @$input; - @$input = @ok if !$dry; - $blocked = { map { ( $_ => 1 ) } @nok }; + $result = []; + map { $self->{_entries}->{$_} ? ( push @$result, $_ ) : ( $dump->{$_} = 1 ); } @$input; } else { # Handle hashref - foreach my $key ( keys %$input ) { - unless ( $self->{_entries}->{$key} ) { - if( ref($input) eq 'HASH' ) { - $blocked->{$key} = $dry ? $input->{$key} : delete $input->{$key}; - } - } - } + $result = {}; + map { $self->{_entries}->{$_} ? ( $result->{$_} = $input->{$_} ) : ( $dump->{$_} = $input->{$_} ); } keys %$input; } - warn 'Koha::AllowList: apply blocked one or more keys' if keys %$blocked && $params->{verbose}; - return $blocked; + map { $params->{dump}->{$_} = $dump->{$_}; } keys %$dump if ref($params->{dump}) eq 'HASH'; + return $result; } =head3 check - my $blocked = $allowlist->check({ input => $a, verbose => 0 }); + my $bool = $allowlist->check({ input => $a, dump => $dump }); - This is just a shortcut for ->apply({ dry_run => 1 }) + This is just a shortcut for running ->apply and checking if nothing was blocked. + Optional dump variable serves the same purpose. + Returns a boolean (1 or empty string). True means that all input is allowed, + but only if there was input. =cut sub check { my ( $self, $params ) = @_; - return $self->apply({ %$params, dry_run => 1 }); + my $dump = {}; + my $rv = $self->apply({ %$params, dump => $dump }); + map { $params->{dump}->{$_} = $dump->{$_}; } keys %$dump if ref($params->{dump}) eq 'HASH'; + return $rv ? keys %$dump == 0 : q{}; } -=head3 load +=head3 reset - my $fields = $self->load; + $allowlist->reset; - Loads default allow list entries. - - This routine should be overridden when subclassing. + Restore default entries. =cut -sub load { +sub reset { my ( $self ) = @_; - $self->add( @$defaults ); + $self->{_entries} = {}; + $self->add( @{$self->{_defaults}} ); } -=head3 unload +=head3 keys - $allowlist->unload; + @keys = $allowlist->keys; - Clear all entries. + Return the allow list entries. =cut -sub unload { +sub keys { my ( $self ) = @_; - $self->{_entries} = {}; + return sort keys %{$self->{_entries}}; } 1; diff --git a/t/Koha/AllowList.t b/t/Koha/AllowList.t index c930709324..ace4d96404 100755 --- a/t/Koha/AllowList.t +++ b/t/Koha/AllowList.t @@ -16,49 +16,60 @@ # along with Koha; if not, see . use Modern::Perl; +use Data::Dumper qw( Dumper ); use Test::More tests => 1; use Koha::AllowList; subtest 'AllowList' => sub { - plan tests => 13; + plan tests => 17; - my $allowlist = Koha::AllowList->new; - $allowlist->load; + my $defaults = [ 'col1' ]; + my $allowlist = Koha::AllowList->new({ defaults => $defaults }); is( $allowlist->apply, undef, 'No input returns undef' ); - is( %{$allowlist->apply({ input => {} })}, 0, 'Empty hash returns nothing blocked' ); + is( %{$allowlist->apply({ input => {} })}, 0, 'Empty hash returns empty hash' ); - # Test return from apply on hashrefs - my @input = ( col1 => 1, col2 => 2, col3 => 3, col4 => 4 ); - my $blocked = $allowlist->apply({ input => { @input } }); + # Test apply on hashrefs + my $input = { col1 => 1, col2 => 2, col3 => 3, col4 => 4 }; + $allowlist->remove('col1'); # results in empty allow list + my $blocked = {}; + $allowlist->apply({ input => $input, dump => $blocked }); is( keys %$blocked, 4, 'Empty list blocks all' ); - $allowlist->add( 'col1' ); - $blocked = $allowlist->apply({ input => { @input } }); + $allowlist->reset; # Back to col1 + $blocked = {}; + $allowlist->apply({ input => $input, dump => $blocked }); is( keys %$blocked, 3, 'One field allowed' ); is( $blocked->{col1}, undef, 'And that is col1' ); - $allowlist->remove( 'col1' ); - $blocked = $allowlist->apply({ input => { @input } }); - is( keys %$blocked, 4, 'Back to 4 after removing col1' ); $allowlist->add( 'col2' ); - $allowlist->unload; - $blocked = $allowlist->apply({ input => { @input } }); - is( keys %$blocked, 4, 'Same after unloading' ); + $blocked = {}; + my $result = $allowlist->apply({ input => $input, dump => $blocked }); + is( keys %$blocked, 2, 'Two fields blocked' ); + is( keys %$result, 2, 'Two fields allowed' ); + is( $result->{col2}, 2, 'Check a result' ); + $allowlist->reset; # Undo col2 + $blocked = {}; + $allowlist->apply({ input => $input, dump => $blocked }); + is( keys %$blocked, 3, 'Back to three blocked' ); - # Test dry run - my $input2 = { col1 => 1, col2 => 2, col3 => 3, col4 => 4 }; + # Test check method + $blocked = {}; + $input = { col1 => 1, col2 => 2, col3 => 3, col4 => 4 }; $allowlist->add( 'col1', 'col3' ); - $blocked = $allowlist->check({ input => $input2 }); - is( keys %$blocked, 2, 'Expected two entries' ); - is( keys %$input2, 4, 'Expected no change in input2' ); - $blocked = $allowlist->apply({ input => $input2 }); - is( keys %$input2, 2, 'Apply removed entries now' ); + is( $allowlist->check({ input => $input, dump => $blocked }), q{}, 'Check returns false' ); + is( keys %$blocked, 2, 'Dumped 2 entries' ); + $allowlist->add( 'col2', 'col4' ); + is( $allowlist->check({ input => $input }), 1, 'Check returns true' ); - # Test array - @input = ( 'col1', 'col2', 'col3', 'col4', 'col5' ); - $blocked = $allowlist->check({ input => [ @input ] }); + # Test on arrays + $allowlist->remove( 'col2', 'col3' ); + $input = [ 'col1', 'col2', 'col3', 'col4', 'col5' ]; + $blocked = {}; + $result = $allowlist->apply({ input => $input, dump => $blocked }); + is( @$result, 2, 'Expected 2 allowed entries' ); + is( ( grep { $_ eq 'col1' } @$result ), 1, 'col1 allowed' ); is( keys %$blocked, 3, 'Expected three blocked entries' ); - $input2 = [ @input ]; - $blocked = $allowlist->apply({ input => $input2 }); - is( keys %$blocked, 3, 'Expected three blocked entries again' ); - is( @$input2, 2, 'Expect two entries now in input2' ); + is( $blocked->{col2}, 1, 'col2 blocked' ); + + # Test keys method + is( join(',', $allowlist->keys), 'col1,col4', 'List current entries' ); }; -- 2.20.1