Bugzilla – Attachment 126115 Details for
Bug 28948
Add a /public counterpart for the libraries REST endpoints
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28948: (QA follow-up) Convert to allow-list
Bug-28948-QA-follow-up-Convert-to-allow-list.patch (text/plain), 11.50 KB, created by
Martin Renvoize (ashimema)
on 2021-10-12 16:10:39 UTC
(
hide
)
Description:
Bug 28948: (QA follow-up) Convert to allow-list
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2021-10-12 16:10:39 UTC
Size:
11.50 KB
patch
obsolete
>From 2f6910a59a7be057b93390e180ca7f209e681eaf Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >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 <martin.renvoize@ptfs-europe.com> >--- > 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.20.1
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=126115">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 28948
:
124479
|
124480
|
124481
|
124482
|
124483
|
124749
|
124911
|
124914
|
125901
|
125902
|
125903
|
125904
|
125905
|
125906
|
125907
|
125908
|
125911
|
125912
|
126111
|
126112
|
126113
|
126114
|
126115
|
126116
|
126117
|
126118
|
126119
|
126120
|
126121
|
126122
|
126123
|
126124
|
126125
|
126126
|
127054
|
127055