Bugzilla – Attachment 124731 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: Centralize the allow list
Bug-28935-Centralize-the-allow-list.patch (text/plain), 12.26 KB, created by
Martin Renvoize (ashimema)
on 2021-09-10 08:52:35 UTC
(
hide
)
Description:
Bug 28935: Centralize the allow list
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2021-09-10 08:52:35 UTC
Size:
12.26 KB
patch
obsolete
>From dd2bbfdabfd37c10e8f98b266ac1956d67ed9348 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Wed, 1 Sep 2021 10:41:21 +0200 >Subject: [PATCH] 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. > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Signed-off-by: Nick Clemens <nick@bywatersolutions.com> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > 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
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