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 <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

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};

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 ) {
+        foreach my $attr (@all_attrs) {
-            ok( exists $privileged_representation->{$privileged_attr},
+            my $mapped = exists $mapping->{$attr} ? $mapping->{$attr} : $attr;
-                "Attribute $privileged_attr' is present" );
+            if ( defined($mapped) ) {
-            ok( !exists $unprivileged_representation->{$privileged_attr},
+                ok(
-                "Attribute '$privileged_attr' is not present" );
+                    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"
+                );
+            }
         }
     };
 

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_1->branchcode, $library_2->branchcode
-                        $library_3->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 ) {
+            foreach my $attr (@all_attrs) {
-                ok( exists $privileged_representation->{$privileged_attr},
+                my $mapped = exists $mapping->{$attr} ? $mapping->{$attr} : $attr;
-                    "Attribute $privileged_attr' is present" );
+                if ( defined($mapped) ) {
-                ok( !exists $unprivileged_representation->{$privileged_attr},
+                    ok(
-                    "Attribute '$privileged_attr' is not present" );
+                        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"
+                    );
+                }
             }
         }
     };