Attachment 124316 Details for Bug 28929 – [Alternate] Bug 28929: Filter user input through allowlist for patrons

[patch] [Alternate] Bug 28929: Filter user input through allowlist for patrons

Alternate-Bug-28929-Filter-user-input-through-allo.patch (text/plain), 11.30 KB, created by David Cook on 2021-09-01 06:46:37 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Cook

Created: 2021-09-01 06:46:37 UTC

Size: 11.30 KB

patch

obsolete

>From 8528009910534ca4c29342265abc2e993bb12f7d 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] [Alternate] Bug 28929: 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
>---
> Koha/Patron/Allowlist.pm        | 54 +++++++++++++++++++++++++++++++
> Koha/Patron/Allowlist/Public.pm | 65 ++++++++++++++++++++++++++++++++++++++
> Koha/Patron/Allowlist/Staff.pm  | 70 +++++++++++++++++++++++++++++++++++++++++
> members/memberentry.pl          |  6 ++++
> opac/opac-memberentry.pl        | 12 ++++++-
> t/Koha/Patron/Allowlist.t       | 48 ++++++++++++++++++++++++++++
> 6 files changed, 254 insertions(+), 1 deletion(-)
> create mode 100644 Koha/Patron/Allowlist.pm
> create mode 100644 Koha/Patron/Allowlist/Public.pm
> create mode 100644 Koha/Patron/Allowlist/Staff.pm
> create mode 100644 t/Koha/Patron/Allowlist.t
>
>diff --git a/Koha/Patron/Allowlist.pm b/Koha/Patron/Allowlist.pm
>new file mode 100644
>index 0000000000..bb01a071be
>--- /dev/null
>+++ b/Koha/Patron/Allowlist.pm
>@@ -0,0 +1,54 @@
>+package Koha::Patron::Allowlist;
>+
>+use Modern::Perl;
>+
>+=head1 NAME
>+
>+Koha::Patron::Allowlist - Allowlist implementation for Koha::Patron 
>+
>+=head1 API
>+
>+=head2 Class Methods
>+
>+=head3 new
>+
>+=cut
>+
>+sub new {
>+    my ($class, $args) = @_;
>+    $args = {} unless defined $args;
>+    my $self = bless ($args, $class);
>+    $self->{ui_fields} = $self->_get_ui_fields() unless $self->{ui_fields};
>+    return $self;
>+}
>+
>+=head3 apply
>+
>+Apply an allowlist to input data
>+
>+=cut
>+
>+sub apply {
>+    my ( $self, $params ) = @_;
>+    my $blocked = {};
>+    my $ui_fields = $self->{ui_fields};
>+    if ($ui_fields && ref $ui_fields eq 'HASH'){
>+        my $input = $params->{input};
>+        if ( $input && ref $input eq 'HASH' ){
>+            my @keys = keys %$input;
>+            foreach my $key (@keys){
>+                if ( ! $ui_fields->{ $key } ){
>+                    #NOTE: We capture the deleted data so that it can be used for logging purposes
>+                    $blocked->{$key} = delete $input->{ $key };
>+                }
>+            }
>+        }
>+    }
>+    return $blocked;
>+}
>+
>+sub _get_ui_fields {
>+    return undef;
>+}
>+
>+1;
>diff --git a/Koha/Patron/Allowlist/Public.pm b/Koha/Patron/Allowlist/Public.pm
>new file mode 100644
>index 0000000000..b63daf16d3
>--- /dev/null
>+++ b/Koha/Patron/Allowlist/Public.pm
>@@ -0,0 +1,65 @@
>+package Koha::Patron::Allowlist::Public;
>+
>+use parent 'Koha::Patron::Allowlist';
>+
>+sub _get_ui_fields {
>+    my $ui_fields = {
>+        'cardnumber' => 1,
>+        'surname' => 1,
>+        'firstname' => 1,
>+        'title' => 1,
>+        'othernames' => 1,
>+        'initials' => 1,
>+        'streetnumber' => 1,
>+        'streettype' => 1,
>+        'address' => 1,
>+        'address2' => 1,
>+        'city' => 1,
>+        'state' => 1,
>+        'zipcode' => 1,
>+        'country' => 1,
>+        'email' => 1,
>+        'phone' => 1,
>+        'mobile' => 1,
>+        'fax' => 1,
>+        'emailpro' => 1,
>+        'phonepro' => 1,
>+        'B_streetnumber' => 1,
>+        'B_streettype' => 1,
>+        'B_address' => 1,
>+        'B_address2' => 1,
>+        'B_city' => 1,
>+        'B_state' => 1,
>+        'B_zipcode' => 1,
>+        'B_country' => 1,
>+        'B_email' => 1,
>+        'B_phone' => 1,
>+        'dateofbirth' => 1,
>+        'dateenrolled' => undef,
>+        'dateexpiry' => undef,
>+        'branchcode' => 1,
>+        'categorycode' => 1,
>+        'contactname' => 1,
>+        'contactfirstname' => 1,
>+        'borrowernotes' => undef,
>+        'sex' => 1,
>+        'password' => 1,
>+        'userid' => 1,
>+        'opacnote' => undef,
>+        'contactnote' => 1,
>+        'altcontactfirstname' => 1,
>+        'altcontactsurname' => 1,
>+        'altcontactaddress1' => 1,
>+        'altcontactaddress2' => 1,
>+        'altcontactaddress3' => 1,
>+        'altcontactstate' => 1,
>+        'altcontactzipcode' => 1,
>+        'altcontactcountry' => 1,
>+        'altcontactphone' => 1,
>+        'smsalertnumber' => 1,
>+        'primary_contact_method' => 1,
>+    };
>+    return $ui_fields;
>+}
>+
>+1;
>diff --git a/Koha/Patron/Allowlist/Staff.pm b/Koha/Patron/Allowlist/Staff.pm
>new file mode 100644
>index 0000000000..2135c6ae83
>--- /dev/null
>+++ b/Koha/Patron/Allowlist/Staff.pm
>@@ -0,0 +1,70 @@
>+package Koha::Patron::Allowlist::Staff;
>+
>+use parent 'Koha::Patron::Allowlist';
>+
>+sub _get_ui_fields {
>+    my $ui_fields = {
>+        'cardnumber' => 1,
>+        'surname' => 1,
>+        'firstname' => 1,
>+        'title' => 1,
>+        'othernames' => 1,
>+        'initials' => 1,
>+        'streetnumber' => 1,
>+        'streettype' => 1,
>+        'address' => 1,
>+        'address2' => 1,
>+        'city' => 1,
>+        'state' => 1,
>+        'zipcode' => 1,
>+        'country' => 1,
>+        'email' => 1,
>+        'phone' => 1,
>+        'mobile' => 1,
>+        'fax' => 1,
>+        'emailpro' => 1,
>+        'phonepro' => 1,
>+        'B_streetnumber' => 1,
>+        'B_streettype' => 1,
>+        'B_address' => 1,
>+        'B_address2' => 1,
>+        'B_city' => 1,
>+        'B_state' => 1,
>+        'B_zipcode' => 1,
>+        'B_country' => 1,
>+        'B_email' => 1,
>+        'B_phone' => 1,
>+        'dateofbirth' => 1,
>+        'dateenrolled' => 1,
>+        'dateexpiry' => 1,
>+        'branchcode' => 1,
>+        'categorycode' => 1,
>+        'contactname' => 1,
>+        'contactfirstname' => 1,
>+        'borrowernotes' => 1,
>+        'sex' => 1,
>+        'password' => 1,
>+        'userid' => 1,
>+        'opacnote' => 1,
>+        'contactnote' => 1,
>+        'altcontactfirstname' => 1,
>+        'altcontactsurname' => 1,
>+        'altcontactaddress1' => 1,
>+        'altcontactaddress2' => 1,
>+        'altcontactaddress3' => 1,
>+        'altcontactstate' => 1,
>+        'altcontactzipcode' => 1,
>+        'altcontactcountry' => 1,
>+        'altcontactphone' => 1,
>+        'smsalertnumber' => 1,
>+        'primary_contact_method' => 1,
>+        'autorenew_checkouts' => 1,
>+        'sort1' => 1,
>+        'sort2' => 1,
>+        'gonenoaddress' => 1,
>+        'lost' => 1,
>+    };
>+    return $ui_fields;
>+}
>+
>+1;
>diff --git a/members/memberentry.pl b/members/memberentry.pl
>index 5844b9c3f9..8e20372334 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::Staff->new();
>+
> 
> my $input = CGI->new;
> my %data;
>@@ -429,6 +433,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 8945cc339b..16c422f609 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::Public;
>+
>+my $ui_allowlist = Koha::Patron::Allowlist::Public->new();
>+
> my $cgi = CGI->new;
> my $dbh = C4::Context->dbh;
> 
>@@ -127,7 +132,11 @@ 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'};
>+
>+    $ui_allowlist->apply({ input => \%borrower });
>+
>     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 +227,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 +274,8 @@ elsif ( $action eq 'update' ) {
>     # Send back the data to the template
>     %borrower = ( %$borrower, %borrower );
> 
>+    $ui_allowlist->apply({ input => \%borrower });
>+
>     if (@empty_mandatory_fields || @$invalidformfields) {
>         $template->param(
>             empty_mandatory_fields => \@empty_mandatory_fields,
>diff --git a/t/Koha/Patron/Allowlist.t b/t/Koha/Patron/Allowlist.t
>new file mode 100644
>index 0000000000..7a1cd7e4cf
>--- /dev/null
>+++ b/t/Koha/Patron/Allowlist.t
>@@ -0,0 +1,48 @@
>+#!/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 t::lib::Mocks;
>+
>+use_ok('Koha::Patron::Allowlist::Staff');
>+use_ok('Koha::Patron::Allowlist::Public');
>+
>+subtest 'Staff Allowlist' => sub {
>+    plan tests => 3;
>+    my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' };
>+    my $allowlist = Koha::Patron::Allowlist::Staff->new();
>+    $allowlist->apply({ input => $input, });
>+    ok( $input->{firstname} eq 'test firstname', 'firstname preserved' );
>+    ok( $input->{surname} eq 'test surname', 'surname preserved' );
>+    is( $input->{flags}, undef, 'flags filtered' );
>+};
>+
>+subtest 'Public Allowlist' => sub {
>+    plan tests => 3;
>+    my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' };
>+    my $allowlist = Koha::Patron::Allowlist::Public->new();
>+    $allowlist->apply({ input => $input, });
>+    ok( $input->{firstname} eq 'test firstname', 'firstname preserved' );
>+    ok( $input->{surname} eq 'test surname', 'surname preserved' );
>+    is( $input->{flags}, undef, 'flags filtered' );
>+};
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 28929: 124308 | 124310 | 124315 | 124316 | 124317 | 124323 | 124325 | 124364 | 124365 | 124366 | 124405 | 124406 | 124407 | 124683 | 124684 | 124685