@@ -, +, @@ --- Koha/Allowlist.pm | 93 +++++++++++++----------- Koha/Patron/Allowlist.pm | 65 ++--------------- t/db_dependent/Koha/Patron/Allowlist.t | 97 ++++++-------------------- 3 files changed, 82 insertions(+), 173 deletions(-) --- a/Koha/Allowlist.pm +++ a/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,79 @@ sub new { ( $args->{interface} && $args->{interface} eq 'staff' ) ? 'staff' : 'opac'; + $self->{_entries} = {}; + $self->_get_entries if $self->can('_get_entries'); return $self; } -=head3 apply +=head3 add - my $ui_fields = Koha::Allowlist->new({ interface => 'staff' })->apply({ input => $hashref, additional_deny_list => [qw( list of fields )] }); + $list->add( 'column1', { column2 => { staff => 1 }} ); +=cut -Apply an allowlist to input data. +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 + + $list->remove( 'column3', 'column4' ); =cut -sub apply { - my ( $self, $params ) = @_; +sub remove { + my ( $self, @args ) = @_; + foreach my $arg ( @args ) { + delete $self->{_entries}->{$arg}; + } +} - my $input = $params->{input}; - my $additional_deny_list = $params->{additional_deny_list} || []; +=head3 apply - my $blocked = {}; - my $ui_fields = { map { $_ => 1 } $self->get_ui_fields() }; - return unless $ui_fields and %$ui_fields; + my $blocked = $allowlist->apply({ input => $hashref, verbose => 1 }); - delete $ui_fields->{$_} for @$additional_deny_list; + Apply an allowlist to input data. Note that we modify the $hashref argument! + The verbose flag controls the warn statement. - return unless $input and %$input; + Returns a hash of blocked entries. Might be useful for logging purposes? - 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 - $blocked->{$key} = delete $input->{ $key }; - } - } +=cut - 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"; +sub apply { + my ( $self, $params ) = @_; + my $input = $params->{input} or return; + + 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}; + # TODO Pass details and caller to Koha::Logger ? return $blocked; } -=head3 get_ui_fields +=head3 _get_entries + + my $fields = $self->_get_entries; - my $ui_fields = $self->get_ui_fields(); - my $ui_fields = Koha::Patron::Allowlist::Public->new->get_ui_fields + This routine should be overridden when subclassing. =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 _get_entries { + my ( $self ) = @_; + # You could do something here like: + # $self->add( 'myfield' ); } 1; --- a/Koha/Patron/Allowlist.pm +++ a/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 _get_entries { + 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; --- a/t/db_dependent/Koha/Patron/Allowlist.t +++ a/t/db_dependent/Koha/Patron/Allowlist.t @@ -16,86 +16,33 @@ # 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 Test::More tests => 1; use Koha::Patron::Allowlist; -use t::lib::Mocks; +subtest 'Patron::Allowlist' => sub { + plan tests => 12; -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 $input = { firstname => 'test firstname', surname => 'test surname', flags => '1', gonenoaddress => 1, unexist => 2 }; 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}; + my $blocked = $allowlist->apply({ input => $input }); - is( $input->{firstname}, 'test firstname', 'firstname preserved' ); - is( $input->{surname}, 'test surname', 'surname preserved' ); + 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' ); -}; - -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' ); - + 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; + $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' ); }; + +#subtest 'Additional adds or removes' => sub { +# --