From aa3e10de2e656b011a04cd06267dc7ca58c629d1 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 1 Sep 2021 06:31:00 +0000 Subject: [PATCH] Bug 28935: Filter user input through allow list Content-Type: text/plain; charset=utf-8 This is a squashed commit of earlier work and a starting point for some further proposed adjustments. Bug 28935: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass Bug 28935: Centralize the allow list We want to make sure developpers won't forget to update the allow list and that we get discrepancy between the list. This patch suggests to have: 1. A global "all fields" list which must match the borrowers table columns 2. A global deny list to apply on top of this list. This deny list is the list of attributes we want to reject from patron's edition from both staff and OPAC interfaces 3. A specific deny list per interface Moreover there is now a warning when a user edit an attribute part of the deny list. Bug 28935: Enforce *UnwantedField sysprefs I didn't recreate the problem on selfmod but it existed for selfreg. Create 2 attributes, 2 visible at the OPAC but only 1 editable. Self reg and modify the input hidden value. Bug 28935: Allow gonenoaddress, lost and relationship for staff Bug 28935: Move logic to Koha::Allowlist for reusability Bug 28935: Take borrowernumber from the original input newdata is been modified, and filtered. We want to pick the original value of borrowernumber. --- 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 diff --git a/Koha/Allowlist.pm b/Koha/Allowlist.pm new file mode 100644 index 0000000000..d0c67b7758 --- /dev/null +++ b/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; diff --git a/Koha/Patron/Allowlist.pm b/Koha/Patron/Allowlist.pm new file mode 100644 index 0000000000..259d61c928 --- /dev/null +++ b/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; diff --git a/members/memberentry.pl b/members/memberentry.pl index c9460583af..d58e7479cd 100755 --- a/members/memberentry.pl +++ b/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')){ diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index c940a6e1af..3c3d9434ae 100755 --- a/opac/opac-memberentry.pl +++ b/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, diff --git a/t/db_dependent/Koha/Patron/Allowlist.t b/t/db_dependent/Koha/Patron/Allowlist.t new file mode 100644 index 0000000000..edec69c678 --- /dev/null +++ b/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' ); + +}; -- 2.20.1