From 5eba587adfc74eec74e0311186b9f50d709d81c2 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 1 Sep 2021 10:41:21 +0200 Subject: [PATCH] Bug 28929: 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. --- Koha/Patron/Allowlist.pm | 160 +++++++++++++++++-- Koha/Patron/Allowlist/Public.pm | 69 ++------ Koha/Patron/Allowlist/Staff.pm | 65 +------- t/{ => db_dependent}/Koha/Patron/Allowlist.t | 36 +++-- 4 files changed, 186 insertions(+), 144 deletions(-) rename t/{ => db_dependent}/Koha/Patron/Allowlist.t (59%) diff --git a/Koha/Patron/Allowlist.pm b/Koha/Patron/Allowlist.pm index bb01a071be3..11587bf8c0e 100644 --- a/Koha/Patron/Allowlist.pm +++ b/Koha/Patron/Allowlist.pm @@ -1,6 +1,7 @@ package Koha::Patron::Allowlist; use Modern::Perl; +use Array::Utils qw( array_minus ); =head1 NAME @@ -18,37 +19,168 @@ 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}; + $self->{ui_fields} = { map { $_ => 1 } $self->get_ui_fields() }; return $self; } =head3 apply -Apply an allowlist to input data + my $ui_fields = Koha::Patron::Allowlist::Public->new->apply({ input => $hashref }); + +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 unless $ui_fields and %$ui_fields; + + my $input = $params->{input}; + 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; } -sub _get_ui_fields { - return undef; +=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->_deny_list(); + my @global_allow_list = array_minus @all_fields, @global_deny_list; + return array_minus @global_allow_list, @deny_list; +} + +# 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 + gonenoaddress + lost + debarred + debarredcomment + relationship + flags + privacy + privacy_guarantor_fines + privacy_guarantor_checkouts + checkprevcheckout + updated_on + lastseen + lang + login_attempts + overdrive_auth_token + anonymized + ); } 1; diff --git a/Koha/Patron/Allowlist/Public.pm b/Koha/Patron/Allowlist/Public.pm index b63daf16d3b..b0891c164b8 100644 --- a/Koha/Patron/Allowlist/Public.pm +++ b/Koha/Patron/Allowlist/Public.pm @@ -2,64 +2,17 @@ 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; +sub _deny_list { + return qw( + dateenrolled + dateexpiry + borrowernotes + opacnote + sort1 + sort2 + sms_provider_id + autorenew_checkouts + ); } 1; diff --git a/Koha/Patron/Allowlist/Staff.pm b/Koha/Patron/Allowlist/Staff.pm index 2135c6ae83b..906e167d82c 100644 --- a/Koha/Patron/Allowlist/Staff.pm +++ b/Koha/Patron/Allowlist/Staff.pm @@ -2,69 +2,8 @@ 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; +sub _deny_list { + return (); } 1; diff --git a/t/Koha/Patron/Allowlist.t b/t/db_dependent/Koha/Patron/Allowlist.t similarity index 59% rename from t/Koha/Patron/Allowlist.t rename to t/db_dependent/Koha/Patron/Allowlist.t index 7a1cd7e4cf2..b8f21096ac2 100644 --- a/t/Koha/Patron/Allowlist.t +++ b/t/db_dependent/Koha/Patron/Allowlist.t @@ -17,32 +17,50 @@ use Modern::Perl; -use Test::More tests => 4; +use Test::More tests => 5; use Test::MockModule; use Test::Exception; +use Test::Warn; + +use Koha::Patrons; +use Koha::Patron::Allowlist; use t::lib::Mocks; use_ok('Koha::Patron::Allowlist::Staff'); use_ok('Koha::Patron::Allowlist::Public'); +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 => 3; + plan tests => 4; 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' ); + 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 => 3; + plan tests => 4; 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' ); + 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' ); }; -- 2.25.1