From 44c02d7dbadd7d63da8da1a08630de5114cda74a Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Fri, 3 Sep 2021 08:23:31 +0000 Subject: [PATCH] Bug 28935: Rename get_ui_fields, introduce add and remove Content-Type: text/plain; charset=utf-8 Also move test to t/Koha/Allowlist.t --- Koha/Allowlist.pm | 104 +++++++++++++++---------- Koha/Patron/Allowlist.pm | 65 ++-------------- t/Koha/Allowlist.t | 73 +++++++++++++++++ t/db_dependent/Koha/Patron/Allowlist.t | 101 ------------------------ 4 files changed, 145 insertions(+), 198 deletions(-) create mode 100755 t/Koha/Allowlist.t delete mode 100755 t/db_dependent/Koha/Patron/Allowlist.t diff --git a/Koha/Allowlist.pm b/Koha/Allowlist.pm index d0c67b7758..6f385ba23d 100644 --- a/Koha/Allowlist.pm +++ b/Koha/Allowlist.pm @@ -1,7 +1,6 @@ package Koha::Allowlist; use Modern::Perl; -use Array::Utils qw( array_minus ); =head1 NAME @@ -11,13 +10,13 @@ Koha::Allowlist - Allowlist implementation base class =head2 Class Methods - my $allow_list Koha::Allowlist->new({ interface => 'staff' }); - =head3 new -Constructor. + my $allow_list = Koha::Allowlist->new({ interface => 'staff' }); + + Constructor. -Interface must be 'staff' or 'opac' - default to 'opac' + Interface must be 'staff' or 'opac' - defaults to 'opac' =cut @@ -29,65 +28,92 @@ sub new { ( $args->{interface} && $args->{interface} eq 'staff' ) ? 'staff' : 'opac'; + $self->{_entries} = {}; return $self; } -=head3 apply +=head3 add + + $list->add( 'column1', { column2 => { staff => 1 }} ); - my $ui_fields = Koha::Allowlist->new({ interface => 'staff' })->apply({ input => $hashref, additional_deny_list => [qw( list of fields )] }); +=cut + +sub add { + my ( $self, @args ) = @_; + foreach my $arg ( @args ) { + if( !ref($arg) ) { + $self->{_entries}->{$arg} = 1; + } elsif( ref($arg) eq 'HASH' ) { + $self->{_entries} = { %{$self->{_entries}}, %$arg }; + } + } +} +=head3 remove -Apply an allowlist to input data. + $list->remove( 'column3', 'column4' ); =cut -sub apply { - my ( $self, $params ) = @_; +sub remove { + my ( $self, @args ) = @_; + foreach my $arg ( @args ) { + delete $self->{_entries}->{$arg}; + } +} + +=head3 apply - my $input = $params->{input}; - my $additional_deny_list = $params->{additional_deny_list} || []; + my $blocked = $allowlist->apply({ input => $hashref, verbose => 1 }); - my $blocked = {}; - my $ui_fields = { map { $_ => 1 } $self->get_ui_fields() }; - return unless $ui_fields and %$ui_fields; + Apply an allowlist to input data. Note that we modify the $hashref argument! + The verbose flag controls the warn statement. - delete $ui_fields->{$_} for @$additional_deny_list; + Returns a hash of blocked entries. Might be useful for logging purposes? - return unless $input and %$input; +=cut + +sub apply { + my ( $self, $params ) = @_; + my $input = $params->{input} or return; - my @keys = keys %$input; - foreach my $key (@keys){ - unless ( exists $ui_fields->{ $key } ) { - #NOTE: We capture the deleted data so that it can be used for logging purposes + my $blocked = {}; + foreach my $key ( keys %$input ){ + unless ( exists $self->{_entries}->{$key} ) { $blocked->{$key} = delete $input->{ $key }; } } + warn 'Koha::Allowlist: apply blocked one or more keys' if keys %$blocked && $params->{verbose}; + return $blocked; +} - if ( %$blocked ) { - while ( my ($k, $v)=each %$blocked){ - # FIXME We should raise an exception from here - my @c = caller; - warn sprintf "Forbidden - Tried to modify '%s' with '%s' from %s", $k, $v, "@c"; - } - } +=head3 load - return $blocked; + my $fields = $self->load; + + Loads default allow list entries. + + This routine should be overridden when subclassing. + +=cut + +sub load { + my ( $self ) = @_; + # You could do something here like: + # $self->add( 'myfield' ); } -=head3 get_ui_fields +=head3 unload + + $allowlist->unload; - my $ui_fields = $self->get_ui_fields(); - my $ui_fields = Koha::Patron::Allowlist::Public->new->get_ui_fields + Clear all entries. =cut -sub get_ui_fields { - my ($self) = @_; - my @all_fields = $self->_get_all_fields(); - my @global_deny_list = $self->_global_deny_list(); - my @deny_list = ( $self->{interface} eq 'staff' ) ? $self->_deny_list_staff() : $self->_deny_list_opac(); - my @global_allow_list = array_minus @all_fields, @global_deny_list; - return array_minus @global_allow_list, @deny_list; +sub unload { + my ( $self ) = @_; + $self->{_entries} = {}; } 1; diff --git a/Koha/Patron/Allowlist.pm b/Koha/Patron/Allowlist.pm index 259d61c928..bcf4be1242 100644 --- a/Koha/Patron/Allowlist.pm +++ b/Koha/Patron/Allowlist.pm @@ -1,12 +1,11 @@ package Koha::Patron::Allowlist; +use Modern::Perl; use parent 'Koha::Allowlist'; -# This must not be replaced by Koha::Patrons->columns -# We want developper to not forget to adjust it manually and add new attribute to the deny list if needed -sub _get_all_fields { - return qw( - borrowernumber +sub load { + my ( $self ) = @_; + my @allowed = qw( cardnumber surname firstname @@ -40,26 +39,13 @@ sub _get_all_fields { dateofbirth branchcode categorycode - dateenrolled - dateexpiry - date_renewed - gonenoaddress - lost - debarred - debarredcomment contactname contactfirstname contacttitle - borrowernotes - relationship sex password - flags userid - opacnote contactnote - sort1 - sort2 altcontactfirstname altcontactsurname altcontactaddress1 @@ -70,49 +56,15 @@ sub _get_all_fields { altcontactcountry altcontactphone smsalertnumber - sms_provider_id - privacy - privacy_guarantor_fines - privacy_guarantor_checkouts - checkprevcheckout - updated_on - lastseen - lang - login_attempts - overdrive_auth_token - anonymized - autorenew_checkouts primary_contact_method ); -} - -sub _global_deny_list { - return qw( - borrowernumber - date_renewed - debarred - debarredcomment - flags - privacy - privacy_guarantor_fines - privacy_guarantor_checkouts - checkprevcheckout - updated_on - lastseen - lang - login_attempts - overdrive_auth_token - anonymized - ); -} + $self->add( @allowed ); -sub _deny_list_opac { - return qw( + my @allowed_staff = qw( dateenrolled dateexpiry gonenoaddress lost - borrowernotes relationship opacnote sort1 @@ -120,10 +72,7 @@ sub _deny_list_opac { sms_provider_id autorenew_checkouts ); -} - -sub _deny_list_staff { - return qw(); + $self->add( @allowed_staff ) if $self->{interface} eq 'staff'; } 1; diff --git a/t/Koha/Allowlist.t b/t/Koha/Allowlist.t new file mode 100755 index 0000000000..887e2c8509 --- /dev/null +++ b/t/Koha/Allowlist.t @@ -0,0 +1,73 @@ +#!/usr/bin/perl + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; +use Test::More tests => 2; + +use Koha::Allowlist; +use Koha::Patron::Allowlist; + +subtest 'Allowlist' => sub { + plan tests => 7; + + my $allowlist = Koha::Allowlist->new; + $allowlist->load; + is( $allowlist->apply, undef, 'No input returns undef' ); + is( %{$allowlist->apply({ input => {} })}, 0, 'Empty hash returns nothing blocked' ); + + my @input = ( col1 => 1, col2 => 2, col3 => 3, col4 => 4 ); + my $blocked = $allowlist->apply({ input => { @input } }); + is( keys %$blocked, 4, 'Empty list blocks all' ); + $allowlist->add( 'col1' ); + $blocked = $allowlist->apply({ input => { @input } }); + 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' ); +}; + +subtest 'Patron::Allowlist' => sub { + plan tests => 12; + + my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1', gonenoaddress => 1, unexist => 2 }; + my $allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); + $allowlist->load; + my $blocked = $allowlist->apply({ input => $input }); + + is( $blocked->{unexist}, 2, "Unexist got blocked" ); + is( $input->{unexist}, undef, 'unexist filtered' ); + is( $blocked->{flags}, 1, "Flags got blocked" ); + is( $input->{flags}, undef, 'flags filtered' ); + is( $input->{firstname}, 'test firstname', 'firstname preserved' ); + is( $input->{surname}, 'test surname', 'surname preserved' ); + is( $input->{gonenoaddress}, 1, 'gonenoaddress preserved on staff' ); + + $input = { firstname => 'test firstname', surname => 'test surname', flags => '1', gonenoaddress => 1, unexist => 2 }; + my $opacallowlist = Koha::Patron::Allowlist->new; + $opacallowlist->load; + $blocked = $opacallowlist->apply({ input => $input }); + is( $blocked->{unexist}, 2, "Unexist got blocked on opac too"); + is( $blocked->{flags}, 1, "Flags got blocked on opac too" ); + is( $blocked->{gonenoaddress}, 1, "Gonenoaddress got blocked on opac now" ); + is( $input->{gonenoaddress}, undef, 'gonenoaddress filtered, opac' ); + is( $input->{surname}, 'test surname', 'surname preserved, opac' ); +}; diff --git a/t/db_dependent/Koha/Patron/Allowlist.t b/t/db_dependent/Koha/Patron/Allowlist.t deleted file mode 100755 index edec69c678..0000000000 --- a/t/db_dependent/Koha/Patron/Allowlist.t +++ /dev/null @@ -1,101 +0,0 @@ -#!/usr/bin/perl - -# This file is part of Koha. -# -# Koha is free software; you can redistribute it and/or modify it -# under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 3 of the License, or -# (at your option) any later version. -# -# Koha is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Koha; if not, see . - -use Modern::Perl; - -use Test::More tests => 4; - -use Test::MockModule; -use Test::Exception; -use Test::Warn; - -use Koha::Patrons; -use Koha::Patron::Allowlist; - -use t::lib::Mocks; - -subtest '_get_all_fields' => sub { - my @columns = Koha::Patrons->columns; - is_deeply( - \@columns, - [ Koha::Patron::Allowlist->_get_all_fields ], - 'All DB columns must be listed in _get_all_fields' - ); -}; - -subtest 'Staff Allowlist' => sub { - plan tests => 4; - my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' }; - my $allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); - warning_like { - $allowlist->apply({ input => $input, }); - } qr{Forbidden - Tried to modify 'flags' with '1' from}; - is( $input->{firstname}, 'test firstname', 'firstname preserved' ); - is( $input->{surname}, 'test surname', 'surname preserved' ); - is( $input->{flags}, undef, 'flags filtered' ); -}; - -subtest 'Public Allowlist' => sub { - plan tests => 4; - my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' }; - my $allowlist = Koha::Patron::Allowlist->new(); - warning_like { - $allowlist->apply({ input => $input, }); - } qr{Forbidden - Tried to modify 'flags' with '1' from}; - - is( $input->{firstname}, 'test firstname', 'firstname preserved' ); - is( $input->{surname}, 'test surname', 'surname preserved' ); - is( $input->{flags}, undef, 'flags filtered' ); -}; - -subtest 'additional_deny_list' => sub { - plan tests => 10; - - my $input = { firstname => 'test firstname', surname => 'test surname' }; - my $allowlist = Koha::Patron::Allowlist->new(); - - $allowlist->apply({ - input => $input, - additional_deny_list => [], - }); - - is( $input->{firstname}, 'test firstname', 'firstname preserved' ); - is( $input->{surname}, 'test surname', 'surname filtered' ); - is( $input->{flags}, undef, 'flags filtered' ); - - $allowlist->apply({ - input => $input, - additional_deny_list => ['not_here'], - }); - - is( $input->{firstname}, 'test firstname', 'firstname preserved' ); - is( $input->{surname}, 'test surname', 'surname filtered' ); - is( $input->{flags}, undef, 'flags filtered' ); - - warning_like { - $allowlist->apply({ - input => $input, - additional_deny_list => ['surname'], - }); - } - qr{Forbidden - Tried to modify 'surname' with 'test surname' from}; - - is( $input->{firstname}, 'test firstname', 'firstname preserved' ); - is( $input->{surname}, undef, 'surname filtered' ); - is( $input->{flags}, undef, 'flags filtered' ); - -}; -- 2.20.1