From b46fdf6168a0b11ce174761098cd5752ac8979b1 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 2 Sep 2021 11:37:45 +0200 Subject: [PATCH] Bug 28935: Move logic to Koha::Allowlist for reusability Content-Type: text/plain; charset=utf-8 Signed-off-by: Marcel de Rooy --- Koha/Allowlist.pm | 93 +++++++++++++++++++++++ Koha/Patron/Allowlist.pm | 100 ++++++------------------- Koha/Patron/Allowlist/Public.pm | 21 ------ Koha/Patron/Allowlist/Staff.pm | 9 --- members/memberentry.pl | 2 +- opac/opac-memberentry.pl | 4 +- t/db_dependent/Koha/Patron/Allowlist.t | 11 +-- 7 files changed, 121 insertions(+), 119 deletions(-) create mode 100644 Koha/Allowlist.pm delete mode 100644 Koha/Patron/Allowlist/Public.pm delete mode 100644 Koha/Patron/Allowlist/Staff.pm diff --git a/Koha/Allowlist.pm b/Koha/Allowlist.pm new file mode 100644 index 0000000000..d0c67b7758 --- /dev/null +++ b/Koha/Allowlist.pm @@ -0,0 +1,93 @@ +package Koha::Allowlist; + +use Modern::Perl; +use Array::Utils qw( array_minus ); + +=head1 NAME + +Koha::Allowlist - Allowlist implementation base class + +=head1 API + +=head2 Class Methods + + my $allow_list Koha::Allowlist->new({ interface => 'staff' }); + +=head3 new + +Constructor. + +Interface must be 'staff' or 'opac' - default to 'opac' + +=cut + +sub new { + my ($class, $args) = @_; + $args = {} unless defined $args; + my $self = bless ($args, $class); + $self->{interface} = + ( $args->{interface} && $args->{interface} eq 'staff' ) + ? 'staff' + : 'opac'; + return $self; +} + +=head3 apply + + my $ui_fields = Koha::Allowlist->new({ interface => 'staff' })->apply({ input => $hashref, additional_deny_list => [qw( list of fields )] }); + + +Apply an allowlist to input data. + +=cut + +sub apply { + my ( $self, $params ) = @_; + + my $input = $params->{input}; + my $additional_deny_list = $params->{additional_deny_list} || []; + + my $blocked = {}; + my $ui_fields = { map { $_ => 1 } $self->get_ui_fields() }; + return unless $ui_fields and %$ui_fields; + + delete $ui_fields->{$_} for @$additional_deny_list; + + 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; +} + +=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->{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; +} + +1; diff --git a/Koha/Patron/Allowlist.pm b/Koha/Patron/Allowlist.pm index 440462102a..259d61c928 100644 --- a/Koha/Patron/Allowlist.pm +++ b/Koha/Patron/Allowlist.pm @@ -1,84 +1,6 @@ package Koha::Patron::Allowlist; -use Modern::Perl; -use Array::Utils qw( array_minus ); - -=head1 NAME - -Koha::Patron::Allowlist - Allowlist implementation for Koha::Patron - -=head1 API - -=head2 Class Methods - -=head3 new - -=cut - -sub new { - my ($class, $args) = @_; - $args = {} unless defined $args; - my $self = bless ($args, $class); - $self->{ui_fields} = { map { $_ => 1 } $self->get_ui_fields() }; - return $self; -} - -=head3 apply - - my $ui_fields = Koha::Patron::Allowlist::Public->new->apply({ input => $hashref, additional_deny_list => [qw( list of fields )] }); - -Apply an allowlist to input data. - -=cut - -sub apply { - my ( $self, $params ) = @_; - - my $input = $params->{input}; - my $additional_deny_list = $params->{additional_deny_list} || []; - - my $blocked = {}; - my $ui_fields = $self->{ui_fields}; - return unless $ui_fields and %$ui_fields; - - delete $ui_fields->{$_} for @$additional_deny_list; - - 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; -} - -=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; -} +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 @@ -184,4 +106,24 @@ sub _global_deny_list { ); } +sub _deny_list_opac { + return qw( + dateenrolled + dateexpiry + gonenoaddress + lost + borrowernotes + relationship + opacnote + sort1 + sort2 + sms_provider_id + autorenew_checkouts + ); +} + +sub _deny_list_staff { + return qw(); +} + 1; diff --git a/Koha/Patron/Allowlist/Public.pm b/Koha/Patron/Allowlist/Public.pm deleted file mode 100644 index 0174277984..0000000000 --- a/Koha/Patron/Allowlist/Public.pm +++ /dev/null @@ -1,21 +0,0 @@ -package Koha::Patron::Allowlist::Public; - -use parent 'Koha::Patron::Allowlist'; - -sub _deny_list { - return qw( - dateenrolled - dateexpiry - gonenoaddress - lost - borrowernotes - relationship - opacnote - sort1 - sort2 - sms_provider_id - autorenew_checkouts - ); -} - -1; diff --git a/Koha/Patron/Allowlist/Staff.pm b/Koha/Patron/Allowlist/Staff.pm deleted file mode 100644 index 906e167d82..0000000000 --- a/Koha/Patron/Allowlist/Staff.pm +++ /dev/null @@ -1,9 +0,0 @@ -package Koha::Patron::Allowlist::Staff; - -use parent 'Koha::Patron::Allowlist'; - -sub _deny_list { - return (); -} - -1; diff --git a/members/memberentry.pl b/members/memberentry.pl index 6b63ee7650..0fc3d2b022 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -48,7 +48,7 @@ use Email::Valid; use Koha::SMS::Providers; use Koha::Patron::Allowlist::Staff; -my $ui_allowlist = Koha::Patron::Allowlist::Staff->new(); +my $ui_allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); my $input = CGI->new; diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index c1f35fe63f..3c3d9434ae 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -46,9 +46,9 @@ use Koha::Patron::Categories; use Koha::Token; use Koha::AuthorisedValues; -use Koha::Patron::Allowlist::Public; +use Koha::Patron::Allowlist; -my $ui_allowlist = Koha::Patron::Allowlist::Public->new(); +my $ui_allowlist = Koha::Patron::Allowlist->new(); my $cgi = CGI->new; my $dbh = C4::Context->dbh; diff --git a/t/db_dependent/Koha/Patron/Allowlist.t b/t/db_dependent/Koha/Patron/Allowlist.t index 5327f54e41..edec69c678 100644 --- a/t/db_dependent/Koha/Patron/Allowlist.t +++ b/t/db_dependent/Koha/Patron/Allowlist.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 6; +use Test::More tests => 4; use Test::MockModule; use Test::Exception; @@ -28,9 +28,6 @@ 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( @@ -43,7 +40,7 @@ subtest '_get_all_fields' => sub { subtest 'Staff Allowlist' => sub { plan tests => 4; my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' }; - my $allowlist = Koha::Patron::Allowlist::Staff->new(); + my $allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); warning_like { $allowlist->apply({ input => $input, }); } qr{Forbidden - Tried to modify 'flags' with '1' from}; @@ -55,7 +52,7 @@ subtest 'Staff Allowlist' => sub { subtest 'Public Allowlist' => sub { plan tests => 4; my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' }; - my $allowlist = Koha::Patron::Allowlist::Public->new(); + my $allowlist = Koha::Patron::Allowlist->new(); warning_like { $allowlist->apply({ input => $input, }); } qr{Forbidden - Tried to modify 'flags' with '1' from}; @@ -69,7 +66,7 @@ subtest 'additional_deny_list' => sub { plan tests => 10; my $input = { firstname => 'test firstname', surname => 'test surname' }; - my $allowlist = Koha::Patron::Allowlist::Public->new(); + my $allowlist = Koha::Patron::Allowlist->new(); $allowlist->apply({ input => $input, -- 2.20.1