@@ -, +, @@ --- 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%) --- a/Koha/Patron/Allowlist.pm +++ a/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; --- a/Koha/Patron/Allowlist/Public.pm +++ a/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; --- a/Koha/Patron/Allowlist/Staff.pm +++ a/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; --- a/t/Koha/Patron/Allowlist.t +++ a/t/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' ); }; --