From d08f9526ea8aa3ad52a5b9f00ea53586dca54544 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 1 Sep 2021 10:41:21 +0200 Subject: [PATCH] Bug 28935: Centralize the allow list Content-Type: text/plain; charset=utf-8 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. Signed-off-by: Marcel de Rooy --- 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 a2dc41c4ae..0e696b1ba4 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 b63daf16d3..b0891c164b 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 2135c6ae83..906e167d82 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 7a1cd7e4cf..b8f21096ac 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.20.1