@@ -, +, @@ --- Koha/Allowlist.pm | 93 ++++++++++++++++++ Koha/Patron/Allowlist.pm | 129 +++++++++++++++++++++++++ members/memberentry.pl | 8 +- opac/opac-memberentry.pl | 14 ++- t/db_dependent/Koha/Patron/Allowlist.t | 101 +++++++++++++++++++ 5 files changed, 343 insertions(+), 2 deletions(-) create mode 100644 Koha/Allowlist.pm create mode 100644 Koha/Patron/Allowlist.pm create mode 100644 t/db_dependent/Koha/Patron/Allowlist.t --- a/Koha/Allowlist.pm +++ a/Koha/Allowlist.pm @@ -0,0 +1,93 @@ +package Koha::Allowlist; + +use Modern::Perl; +use Array::Utils qw( array_minus ); + +=head1 NAME + +Koha::Allowlist - Allowlist implementation base class + +=head1 API + +=head2 Class Methods + + my $allow_list Koha::Allowlist->new({ interface => 'staff' }); + +=head3 new + +Constructor. + +Interface must be 'staff' or 'opac' - default to 'opac' + +=cut + +sub new { + my ($class, $args) = @_; + $args = {} unless defined $args; + my $self = bless ($args, $class); + $self->{interface} = + ( $args->{interface} && $args->{interface} eq 'staff' ) + ? 'staff' + : 'opac'; + return $self; +} + +=head3 apply + + my $ui_fields = Koha::Allowlist->new({ interface => 'staff' })->apply({ input => $hashref, additional_deny_list => [qw( list of fields )] }); + + +Apply an allowlist to input data. + +=cut + +sub apply { + my ( $self, $params ) = @_; + + my $input = $params->{input}; + my $additional_deny_list = $params->{additional_deny_list} || []; + + my $blocked = {}; + my $ui_fields = { map { $_ => 1 } $self->get_ui_fields() }; + return unless $ui_fields and %$ui_fields; + + delete $ui_fields->{$_} for @$additional_deny_list; + + return unless $input and %$input; + + 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 }; + } + } + + 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"; + } + } + + return $blocked; +} + +=head3 get_ui_fields + + my $ui_fields = $self->get_ui_fields(); + my $ui_fields = Koha::Patron::Allowlist::Public->new->get_ui_fields + +=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; +} + +1; --- a/Koha/Patron/Allowlist.pm +++ a/Koha/Patron/Allowlist.pm @@ -0,0 +1,129 @@ +package Koha::Patron::Allowlist; + +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 + cardnumber + surname + firstname + title + othernames + initials + streetnumber + streettype + address + address2 + city + state + zipcode + country + email + phone + mobile + fax + emailpro + phonepro + B_streetnumber + B_streettype + B_address + B_address2 + B_city + B_state + B_zipcode + B_country + B_email + B_phone + 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 + altcontactaddress2 + altcontactaddress3 + altcontactstate + altcontactzipcode + 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 + ); +} + +sub _deny_list_opac { + return qw( + dateenrolled + dateexpiry + gonenoaddress + lost + borrowernotes + relationship + opacnote + sort1 + sort2 + sms_provider_id + autorenew_checkouts + ); +} + +sub _deny_list_staff { + return qw(); +} + +1; --- a/members/memberentry.pl +++ a/members/memberentry.pl @@ -46,6 +46,10 @@ use Koha::Patron::HouseboundRoles; use Koha::Token; use Email::Valid; use Koha::SMS::Providers; +use Koha::Patron::Allowlist::Staff; + +my $ui_allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); + my $input = CGI->new; my %data; @@ -323,7 +327,7 @@ if ($op eq 'save' || $op eq 'insert'){ # If the cardnumber is blank, treat it as null. $newdata{'cardnumber'} = undef if $newdata{'cardnumber'} =~ /^\s*$/; - if (my $error_code = checkcardnumber($newdata{cardnumber},$newdata{borrowernumber})){ + if (my $error_code = checkcardnumber($newdata{cardnumber}, $borrowernumber)){ push @errors, $error_code == 1 ? 'ERROR_cardnumber_already_exists' : $error_code == 2 @@ -430,6 +434,8 @@ if ( defined $sms ) { $newdata{smsalertnumber} = $sms; } +$ui_allowlist->apply({ input => \%newdata }); + ### Error checks should happen before this line. $nok = $nok || scalar(@errors); if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ --- a/opac/opac-memberentry.pl +++ a/opac/opac-memberentry.pl @@ -45,6 +45,11 @@ use Koha::Patron::Modifications; use Koha::Patron::Categories; use Koha::Token; use Koha::AuthorisedValues; + +use Koha::Patron::Allowlist; + +my $ui_allowlist = Koha::Patron::Allowlist->new(); + my $cgi = CGI->new; my $dbh = C4::Context->dbh; @@ -127,7 +132,12 @@ if ( $action eq 'create' ) { my @empty_mandatory_fields = (CheckMandatoryFields( \%borrower, $action ), CheckMandatoryAttributes( \%borrower, $attributes ) ); my $invalidformfields = CheckForInvalidFields(\%borrower); + my $consent_dt = delete $borrower{gdpr_proc_consent}; delete $borrower{'password2'}; + + my @unwanted_fields = split( /\|/, C4::Context->preference('PatronSelfRegistrationBorrowerUnwantedField') || q|| ); + $ui_allowlist->apply({ input => \%borrower, additional_deny_list => \@unwanted_fields }); + my $cardnumber_error_code; if ( !grep { $_ eq 'cardnumber' } @empty_mandatory_fields ) { # No point in checking the cardnumber if it's missing and mandatory, it'll just generate a @@ -218,7 +228,6 @@ if ( $action eq 'create' ) { ); $borrower{password} ||= Koha::AuthUtils::generate_password(Koha::Patron::Categories->find($borrower{categorycode})); - my $consent_dt = delete $borrower{gdpr_proc_consent}; my $patron = Koha::Patron->new( \%borrower )->store; Koha::Patron::Consent->new({ borrowernumber => $patron->borrowernumber, type => 'GDPR_PROCESSING', given_on => $consent_dt })->store if $consent_dt; if ( $patron ) { @@ -266,6 +275,9 @@ elsif ( $action eq 'update' ) { # Send back the data to the template %borrower = ( %$borrower, %borrower ); + my @unwanted_fields = split( /\|/, C4::Context->preference('PatronSelfModificationBorrowerUnwantedField') || q|| ); + $ui_allowlist->apply({ input => \%borrower, additional_deny_list => \@unwanted_fields }); + if (@empty_mandatory_fields || @$invalidformfields) { $template->param( empty_mandatory_fields => \@empty_mandatory_fields, --- a/t/db_dependent/Koha/Patron/Allowlist.t +++ a/t/db_dependent/Koha/Patron/Allowlist.t @@ -0,0 +1,101 @@ +#!/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' ); + +}; --