Bugzilla – Attachment 124501 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: Rename get_ui_fields, introduce add and remove
Bug-28935-Rename-getuifields-introduce-add-and-rem.patch (text/plain), 13.98 KB, created by
Marcel de Rooy
on 2021-09-03 17:24:34 UTC
(
hide
)
Description:
Bug 28935: Rename get_ui_fields, introduce add and remove
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2021-09-03 17:24:34 UTC
Size:
13.98 KB
patch
obsolete
>From afefe9b0796cc6bccde2fe4d84e9035bb5af5aca Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Fri, 3 Sep 2021 08:23:31 +0000 >Subject: [PATCH] Bug 28935: Rename get_ui_fields, introduce add and remove >Content-Type: text/plain; charset=utf-8 > >Also move test to t/Koha/Allowlist.t >And add load, unload methods. > >Test plan: >Run Allowlist.t > >Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >--- > Koha/Allowlist.pm | 101 +++++++++++++++---------- > Koha/Patron/Allowlist.pm | 64 ++-------------- > t/Koha/Allowlist.t | 73 ++++++++++++++++++ > t/db_dependent/Koha/Patron/Allowlist.t | 101 ------------------------- > 4 files changed, 142 insertions(+), 197 deletions(-) > create mode 100755 t/Koha/Allowlist.t > delete mode 100755 t/db_dependent/Koha/Patron/Allowlist.t > >diff --git a/Koha/Allowlist.pm b/Koha/Allowlist.pm >index d0c67b7758..e563dcf3f3 100644 >--- a/Koha/Allowlist.pm >+++ b/Koha/Allowlist.pm >@@ -1,7 +1,6 @@ > package Koha::Allowlist; > > use Modern::Perl; >-use Array::Utils qw( array_minus ); > > =head1 NAME > >@@ -11,13 +10,13 @@ Koha::Allowlist - Allowlist implementation base class > > =head2 Class Methods > >- my $allow_list Koha::Allowlist->new({ interface => 'staff' }); >- > =head3 new > >-Constructor. >+ my $allow_list = Koha::Allowlist->new({ interface => 'staff' }); >+ >+ Constructor. > >-Interface must be 'staff' or 'opac' - default to 'opac' >+ Interface must be 'staff' or 'opac' - defaults to 'opac' > > =cut > >@@ -29,65 +28,89 @@ sub new { > ( $args->{interface} && $args->{interface} eq 'staff' ) > ? 'staff' > : 'opac'; >+ $self->{_entries} = {}; > return $self; > } > >-=head3 apply >+=head3 add >+ >+ $list->add( 'column1', 'column2' ); > >- my $ui_fields = Koha::Allowlist->new({ interface => 'staff' })->apply({ input => $hashref, additional_deny_list => [qw( list of fields )] }); >+=cut >+ >+sub add { >+ my ( $self, @args ) = @_; >+ # TODO Could be extended by passing things like column3 => { staff => 1 } >+ foreach my $arg ( @args ) { >+ $self->{_entries}->{$arg} = 1; >+ } >+} > >+=head3 remove > >-Apply an allowlist to input data. >+ $list->remove( 'column3', 'column4' ); > > =cut > >-sub apply { >- my ( $self, $params ) = @_; >+sub remove { >+ my ( $self, @args ) = @_; >+ foreach my $arg ( @args ) { >+ delete $self->{_entries}->{$arg}; >+ } >+} > >- my $input = $params->{input}; >- my $additional_deny_list = $params->{additional_deny_list} || []; >+=head3 apply > >- my $blocked = {}; >- my $ui_fields = { map { $_ => 1 } $self->get_ui_fields() }; >- return unless $ui_fields and %$ui_fields; >+ my $blocked = $allowlist->apply({ input => $hashref, verbose => 1 }); > >- delete $ui_fields->{$_} for @$additional_deny_list; >+ Apply an allowlist to input data. Note that we modify the $hashref argument! >+ The verbose flag controls the warn statement. > >- return unless $input and %$input; >+ Returns a hash of blocked entries. Might be useful for logging purposes? >+ >+=cut > >- 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 >+sub apply { >+ my ( $self, $params ) = @_; >+ my $input = $params->{input} or return; >+ >+ my $blocked = {}; >+ foreach my $key ( keys %$input ){ >+ unless ( $self->{_entries}->{$key} ) { > $blocked->{$key} = delete $input->{ $key }; > } > } >+ warn 'Koha::Allowlist: apply blocked one or more keys' if keys %$blocked && $params->{verbose}; >+ return $blocked; >+} > >- 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"; >- } >- } >+=head3 load > >- return $blocked; >+ my $fields = $self->load; >+ >+ Loads default allow list entries. >+ >+ This routine should be overridden when subclassing. >+ >+=cut >+ >+sub load { >+ my ( $self ) = @_; >+ # You could do something here like: >+ # $self->add( 'myfield' ); > } > >-=head3 get_ui_fields >+=head3 unload >+ >+ $allowlist->unload; > >- my $ui_fields = $self->get_ui_fields(); >- my $ui_fields = Koha::Patron::Allowlist::Public->new->get_ui_fields >+ Clear all entries. > > =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->{interface} eq 'staff' ) ? $self->_deny_list_staff() : $self->_deny_list_opac(); >- my @global_allow_list = array_minus @all_fields, @global_deny_list; >- return array_minus @global_allow_list, @deny_list; >+sub unload { >+ my ( $self ) = @_; >+ $self->{_entries} = {}; > } > > 1; >diff --git a/Koha/Patron/Allowlist.pm b/Koha/Patron/Allowlist.pm >index 259d61c928..cc712ec084 100644 >--- a/Koha/Patron/Allowlist.pm >+++ b/Koha/Patron/Allowlist.pm >@@ -1,12 +1,11 @@ > package Koha::Patron::Allowlist; > >+use Modern::Perl; > use parent 'Koha::Allowlist'; > >-# 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 >+sub load { >+ my ( $self ) = @_; >+ my @allowed = qw( > cardnumber > surname > firstname >@@ -40,26 +39,13 @@ sub _get_all_fields { > 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 >@@ -70,44 +56,11 @@ sub _get_all_fields { > 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 >- debarred >- debarredcomment >- flags >- privacy >- privacy_guarantor_fines >- privacy_guarantor_checkouts >- checkprevcheckout >- updated_on >- lastseen >- lang >- login_attempts >- overdrive_auth_token >- anonymized >- ); >-} >+ $self->add( @allowed ); > >-sub _deny_list_opac { >- return qw( >+ my @allowed_staff = qw( > dateenrolled > dateexpiry > gonenoaddress >@@ -120,10 +73,7 @@ sub _deny_list_opac { > sms_provider_id > autorenew_checkouts > ); >-} >- >-sub _deny_list_staff { >- return qw(); >+ $self->add( @allowed_staff ) if $self->{interface} eq 'staff'; > } > > 1; >diff --git a/t/Koha/Allowlist.t b/t/Koha/Allowlist.t >new file mode 100755 >index 0000000000..887e2c8509 >--- /dev/null >+++ b/t/Koha/Allowlist.t >@@ -0,0 +1,73 @@ >+#!/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 => 2; >+ >+use Koha::Allowlist; >+use Koha::Patron::Allowlist; >+ >+subtest 'Allowlist' => sub { >+ plan tests => 7; >+ >+ my $allowlist = Koha::Allowlist->new; >+ $allowlist->load; >+ is( $allowlist->apply, undef, 'No input returns undef' ); >+ is( %{$allowlist->apply({ input => {} })}, 0, 'Empty hash returns nothing blocked' ); >+ >+ my @input = ( col1 => 1, col2 => 2, col3 => 3, col4 => 4 ); >+ my $blocked = $allowlist->apply({ input => { @input } }); >+ is( keys %$blocked, 4, 'Empty list blocks all' ); >+ $allowlist->add( 'col1' ); >+ $blocked = $allowlist->apply({ input => { @input } }); >+ is( keys %$blocked, 3, 'One field allowed' ); >+ is( $blocked->{col1}, undef, 'And that is col1' ); >+ $allowlist->remove( 'col1' ); >+ $blocked = $allowlist->apply({ input => { @input } }); >+ is( keys %$blocked, 4, 'Back to 4 after removing col1' ); >+ $allowlist->add( 'col2' ); >+ $allowlist->unload; >+ $blocked = $allowlist->apply({ input => { @input } }); >+ is( keys %$blocked, 4, 'Same after unloading' ); >+}; >+ >+subtest 'Patron::Allowlist' => sub { >+ plan tests => 12; >+ >+ my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1', gonenoaddress => 1, unexist => 2 }; >+ my $allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); >+ $allowlist->load; >+ my $blocked = $allowlist->apply({ input => $input }); >+ >+ is( $blocked->{unexist}, 2, "Unexist got blocked" ); >+ is( $input->{unexist}, undef, 'unexist filtered' ); >+ is( $blocked->{flags}, 1, "Flags got blocked" ); >+ is( $input->{flags}, undef, 'flags filtered' ); >+ is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >+ is( $input->{surname}, 'test surname', 'surname preserved' ); >+ is( $input->{gonenoaddress}, 1, 'gonenoaddress preserved on staff' ); >+ >+ $input = { firstname => 'test firstname', surname => 'test surname', flags => '1', gonenoaddress => 1, unexist => 2 }; >+ my $opacallowlist = Koha::Patron::Allowlist->new; >+ $opacallowlist->load; >+ $blocked = $opacallowlist->apply({ input => $input }); >+ is( $blocked->{unexist}, 2, "Unexist got blocked on opac too"); >+ is( $blocked->{flags}, 1, "Flags got blocked on opac too" ); >+ is( $blocked->{gonenoaddress}, 1, "Gonenoaddress got blocked on opac now" ); >+ is( $input->{gonenoaddress}, undef, 'gonenoaddress filtered, opac' ); >+ is( $input->{surname}, 'test surname', 'surname preserved, opac' ); >+}; >diff --git a/t/db_dependent/Koha/Patron/Allowlist.t b/t/db_dependent/Koha/Patron/Allowlist.t >deleted file mode 100755 >index edec69c678..0000000000 >--- a/t/db_dependent/Koha/Patron/Allowlist.t >+++ /dev/null >@@ -1,101 +0,0 @@ >-#!/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 Test::Warn; >- >-use Koha::Patrons; >-use Koha::Patron::Allowlist; >- >-use t::lib::Mocks; >- >-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 => 4; >- my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' }; >- my $allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); >- 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 => 4; >- my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' }; >- my $allowlist = Koha::Patron::Allowlist->new(); >- 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 'additional_deny_list' => sub { >- plan tests => 10; >- >- my $input = { firstname => 'test firstname', surname => 'test surname' }; >- my $allowlist = Koha::Patron::Allowlist->new(); >- >- $allowlist->apply({ >- input => $input, >- additional_deny_list => [], >- }); >- >- is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >- is( $input->{surname}, 'test surname', 'surname filtered' ); >- is( $input->{flags}, undef, 'flags filtered' ); >- >- $allowlist->apply({ >- input => $input, >- additional_deny_list => ['not_here'], >- }); >- >- is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >- is( $input->{surname}, 'test surname', 'surname filtered' ); >- is( $input->{flags}, undef, 'flags filtered' ); >- >- warning_like { >- $allowlist->apply({ >- input => $input, >- additional_deny_list => ['surname'], >- }); >- } >- qr{Forbidden - Tried to modify 'surname' with 'test surname' from}; >- >- is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >- is( $input->{surname}, undef, 'surname filtered' ); >- 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