Bugzilla – Attachment 124499 Details for
Bug 28935
No filtering on patron's data on member entry pages (OPAC, self registration, staff interface)
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28935: Filter user input through allow list
Bug-28935-Filter-user-input-through-allow-list.patch (text/plain), 14.77 KB, created by
Marcel de Rooy
on 2021-09-03 17:24:24 UTC
(
hide
)
Description:
Bug 28935: Filter user input through allow list
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2021-09-03 17:24:24 UTC
Size:
14.77 KB
patch
obsolete
>From 00590d074f45f370e5334c9169bc4b686b38a660 Mon Sep 17 00:00:00 2001 >From: David Cook <dcook@prosentient.com.au> >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. > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >--- > 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 <http://www.gnu.org/licenses>. >+ >+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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 28935
:
124326
|
124327
|
124350
|
124416
|
124418
|
124422
|
124425
|
124446
|
124447
|
124448
|
124449
|
124450
|
124461
|
124462
|
124499
|
124500
|
124501
|
124502
|
124503
|
124523
|
124524
|
124525
|
124526
|
124527
|
124528
|
124529
|
124530
|
124531
|
124532
|
124640
|
124641
|
124642
|
124643
|
124644
|
124645
|
124646
|
124647
|
124648
|
124649
|
124650
|
124730
|
124731
|
124732
|
124733
|
124734
|
124735
|
124736
|
124737
|
124738
|
124739
|
124740
|
124769
|
124784
|
124832
|
124833
|
124837
|
124838
|
124839
|
124840