From bbb6a02fbe264f17b2bbf1ed29a2b1a1000607c3 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 3 Sep 2021 12:18:03 +0100 Subject: [PATCH] Bug 28948: (QA follow-up) Convert to allow-list This patch converts the code to use an allow-list as aposed to a deny-list. This is more 'fail safe' than requireing maintanence of a deny-list. We also switch to using db fields names for the list as aposed to api mapped names. This way, the list can be re-used for non-api related sanitising if required. Signed-off-by: Martin Renvoize --- Koha/Library.pm | 98 +++++++++++++++++------------------ Koha/Object.pm | 20 +++---- t/db_dependent/Koha/Object.t | 41 ++++++++++++--- t/db_dependent/Koha/Objects.t | 45 ++++++++++++---- 4 files changed, 127 insertions(+), 77 deletions(-) diff --git a/Koha/Library.pm b/Koha/Library.pm index 43fc86d838..48e125529b 100644 --- a/Koha/Library.pm +++ b/Koha/Library.pm @@ -226,57 +226,6 @@ sub cash_registers { return Koha::Cash::Registers->_new_from_dbic( $rs ); } -=head3 to_api_mapping - -This method returns the mapping for representing a Koha::Library object -on the API. - -=cut - -sub to_api_mapping { - return { - branchcode => 'library_id', - branchname => 'name', - branchaddress1 => 'address1', - branchaddress2 => 'address2', - branchaddress3 => 'address3', - branchzip => 'postal_code', - branchcity => 'city', - branchstate => 'state', - branchcountry => 'country', - branchphone => 'phone', - branchfax => 'fax', - branchemail => 'email', - branchillemail => 'illemail', - branchreplyto => 'reply_to_email', - branchreturnpath => 'return_path_email', - branchurl => 'url', - issuing => undef, - branchip => 'ip', - branchnotes => 'notes', - marcorgcode => 'marc_org_code', - }; -} - -=head3 api_privileged_attrs - -This method returns the list of privileged access-only attributes. This is used -by $library->to_api($params) to render the object correctly, based on the passed I<$params>. - -=cut - -sub api_privileged_attrs { - return [ - 'illemail', - 'reply_to_email', - 'return_path_email', - 'ip', - 'notes', - 'marc_org_code' - ]; -} - - =head3 get_hold_libraries Return all libraries (including self) that belong to the same hold groups @@ -317,6 +266,53 @@ sub validate_hold_sibling { ->count > 0; } +=head3 public_read_list + +This method returns the list of publicly readable database fields for both API and UI output purposes + +=cut + +sub public_read_list { + return [ + 'branchcode', 'branchname', 'branchaddress1', + 'branchaddress2', 'branchaddress3', 'branchzip', + 'branchcity', 'branchstate', 'branchcountry', + 'branchfax', 'branchemail', 'branchurl' + ]; +} + +=head3 to_api_mapping + +This method returns the mapping for representing a Koha::Library object +on the API. + +=cut + +sub to_api_mapping { + return { + branchcode => 'library_id', + branchname => 'name', + branchaddress1 => 'address1', + branchaddress2 => 'address2', + branchaddress3 => 'address3', + branchzip => 'postal_code', + branchcity => 'city', + branchstate => 'state', + branchcountry => 'country', + branchphone => 'phone', + branchfax => 'fax', + branchemail => 'email', + branchillemail => 'illemail', + branchreplyto => 'reply_to_email', + branchreturnpath => 'return_path_email', + branchurl => 'url', + issuing => undef, + branchip => 'ip', + branchnotes => 'notes', + marcorgcode => 'marc_org_code', + }; +} + =head2 Internal methods =head3 _type diff --git a/Koha/Object.pm b/Koha/Object.pm index 1c2355aa81..6ebf7870be 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -24,6 +24,7 @@ use Carp qw( croak ); use Mojo::JSON; use Scalar::Util qw( blessed looks_like_number ); use Try::Tiny qw( catch try ); +use List::MoreUtils qw( any ); use Koha::Database; use Koha::Exceptions::Object; @@ -552,6 +553,16 @@ sub to_api { my ( $self, $params ) = @_; my $json_object = $self->TO_JSON; + # Remove forbidden attributes if required + # FIXME: We should eventually require public_read_list in all objects and drop the conditional here. + if ( $params->{public} + and $self->can('public_read_list') ) + { + for my $field ( keys %{$json_object} ) { + delete $json_object->{$field} unless any { $_ eq $field } @{$self->public_read_list}; + } + } + my $to_api_mapping = $self->to_api_mapping; # Rename attributes if there's a mapping @@ -573,15 +584,6 @@ sub to_api { } } - # Remove forbidden attributes if required - if ( $params->{public} - and $self->can('api_privileged_attrs') ) - { - foreach my $privileged_attr ( @{ $self->api_privileged_attrs } ) { - delete $json_object->{$privileged_attr}; - } - } - # Make sure we duplicate the $params variable to avoid # breaking calls in a loop (Koha::Objects->to_api) $params = {%$params}; diff --git a/t/db_dependent/Koha/Object.t b/t/db_dependent/Koha/Object.t index 25643a8bb5..0370969479 100755 --- a/t/db_dependent/Koha/Object.t +++ b/t/db_dependent/Koha/Object.t @@ -375,9 +375,11 @@ subtest "to_api() tests" => sub { subtest 'unprivileged request tests' => sub { - my @privileged_attrs = @{ Koha::Library->api_privileged_attrs }; + my @all_attrs = Koha::Libraries->columns(); + my $public_attrs = { map { $_ => 1 } @{ Koha::Library->public_read_list() } }; + my $mapping = Koha::Library->to_api_mapping; - plan tests => scalar @privileged_attrs * 2; + plan tests => scalar @all_attrs * 2; # Create a sample library my $library = $builder->build_object( { class => 'Koha::Libraries' } ); @@ -385,11 +387,36 @@ subtest "to_api() tests" => sub { my $unprivileged_representation = $library->to_api({ public => 1 }); my $privileged_representation = $library->to_api; - foreach my $privileged_attr ( @privileged_attrs ) { - ok( exists $privileged_representation->{$privileged_attr}, - "Attribute $privileged_attr' is present" ); - ok( !exists $unprivileged_representation->{$privileged_attr}, - "Attribute '$privileged_attr' is not present" ); + foreach my $attr (@all_attrs) { + my $mapped = exists $mapping->{$attr} ? $mapping->{$attr} : $attr; + if ( defined($mapped) ) { + ok( + exists $privileged_representation->{$mapped}, + "Attribute '$attr' is present when privileged" + ); + if ( exists $public_attrs->{$attr} ) { + ok( + exists $unprivileged_representation->{$mapped}, + "Attribute '$attr' is present when public" + ); + } + else { + ok( + !exists $unprivileged_representation->{$mapped}, + "Attribute '$attr' is not present when public" + ); + } + } + else { + ok( + !exists $privileged_representation->{$attr}, + "Unmapped attribute '$attr' is not present when privileged" + ); + ok( + !exists $unprivileged_representation->{$attr}, + "Unmapped attribute '$attr' is not present when public" + ); + } } }; diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t index a3c9c446c6..34385dc7f4 100755 --- a/t/db_dependent/Koha/Objects.t +++ b/t/db_dependent/Koha/Objects.t @@ -400,24 +400,24 @@ subtest "to_api() tests" => sub { subtest 'unprivileged request tests' => sub { - my @privileged_attrs = @{ Koha::Library->api_privileged_attrs }; + my @all_attrs = Koha::Libraries->columns(); + my $public_attrs = { map { $_ => 1 } @{ Koha::Library->public_read_list() } }; + my $mapping = Koha::Library->to_api_mapping; # Create sample libraries my $library_1 = $builder->build_object({ class => 'Koha::Libraries' }); my $library_2 = $builder->build_object({ class => 'Koha::Libraries' }); - my $library_3 = $builder->build_object({ class => 'Koha::Libraries' }); my $libraries = Koha::Libraries->search( { branchcode => { '-in' => [ - $library_1->branchcode, $library_2->branchcode, - $library_3->branchcode + $library_1->branchcode, $library_2->branchcode ] } } ); - plan tests => scalar @privileged_attrs * 2 * $libraries->count; + plan tests => scalar @all_attrs * 2 * $libraries->count; my $libraries_unprivileged_representation = $libraries->to_api({ public => 1 }); my $libraries_privileged_representation = $libraries->to_api(); @@ -425,11 +425,36 @@ subtest "to_api() tests" => sub { for (my $i = 0; $i < $libraries->count; $i++) { my $privileged_representation = $libraries_privileged_representation->[$i]; my $unprivileged_representation = $libraries_unprivileged_representation->[$i]; - foreach my $privileged_attr ( @privileged_attrs ) { - ok( exists $privileged_representation->{$privileged_attr}, - "Attribute $privileged_attr' is present" ); - ok( !exists $unprivileged_representation->{$privileged_attr}, - "Attribute '$privileged_attr' is not present" ); + foreach my $attr (@all_attrs) { + my $mapped = exists $mapping->{$attr} ? $mapping->{$attr} : $attr; + if ( defined($mapped) ) { + ok( + exists $privileged_representation->{$mapped}, + "Attribute '$attr' is present when privileged" + ); + if ( exists $public_attrs->{$attr} ) { + ok( + exists $unprivileged_representation->{$mapped}, + "Attribute '$attr' is present when public" + ); + } + else { + ok( + !exists $unprivileged_representation->{$mapped}, + "Attribute '$attr' is not present when public" + ); + } + } + else { + ok( + !exists $privileged_representation->{$attr}, + "Unmapped attribute '$attr' is not present when privileged" + ); + ok( + !exists $unprivileged_representation->{$attr}, + "Unmapped attribute '$attr' is not present when public" + ); + } } } }; -- 2.33.0