From 3308321387c865edc2d8ccb7e3e0e42e9b69dbba Mon Sep 17 00:00:00 2001 From: Irina Peshinskaya Date: Wed, 21 Mar 2018 10:24:31 +0100 Subject: [PATCH] TT#34015 Clear and stable implementation of early input check Change-Id: I12788583d24f8a02e6e29f04194d8daa0b159efa (cherry picked from commit e946675f4893ba50e0389ff78642b783f7386318) --- .../Panel/Controller/API/PhonebookEntries.pm | 2 +- lib/NGCP/Panel/Role/API.pm | 73 +++++++++++++++---- lib/NGCP/Panel/Role/API/PhonebookEntries.pm | 39 ++++++---- lib/NGCP/Panel/Role/Entities.pm | 11 ++- lib/NGCP/Panel/Role/EntitiesItem.pm | 2 +- 5 files changed, 90 insertions(+), 37 deletions(-) diff --git a/lib/NGCP/Panel/Controller/API/PhonebookEntries.pm b/lib/NGCP/Panel/Controller/API/PhonebookEntries.pm index 42f213b997..0a1b8588cf 100644 --- a/lib/NGCP/Panel/Controller/API/PhonebookEntries.pm +++ b/lib/NGCP/Panel/Controller/API/PhonebookEntries.pm @@ -27,7 +27,7 @@ sub api_description { } sub order_by_cols { - return {name => 'me.name', number => 'me.number', shared => 'me.shared'}; + return {name => 'me.name', number => 'me.number'}; } sub query_params { diff --git a/lib/NGCP/Panel/Role/API.pm b/lib/NGCP/Panel/Role/API.pm index e14278a81c..4ae668f404 100644 --- a/lib/NGCP/Panel/Role/API.pm +++ b/lib/NGCP/Panel/Role/API.pm @@ -18,6 +18,7 @@ use DateTime::Format::RFC3339 qw(); use Types::Standard qw(InstanceOf); use Regexp::Common qw(delimited); # $RE{delimited} use HTTP::Headers::Util qw(split_header_words); +use Data::Compare; use Data::HAL qw(); use Data::HAL::Link qw(); use NGCP::Panel::Utils::ValidateJSON qw(); @@ -30,7 +31,7 @@ use NGCP::Panel::Utils::Journal qw(); sub get_valid_data{ my ($self, %params) = @_; - my ($data,$resource,$special_data_process); + my ($data,$resource,$non_json_data); my $c = $params{c}; my $method = $params{method} // uc($c->request->method); @@ -59,7 +60,7 @@ sub get_valid_data{ return unless $self->require_body($c); $data = $c->stash->{body}; $resource = $c->req->query_params; - $special_data_process = 1; + $non_json_data = 1; } if ($json_media_type eq 'application/json' || @@ -75,10 +76,41 @@ sub get_valid_data{ } return unless $self->get_uploads($c, $json, $params{uploads}, $params{form}); $resource = $json; - $special_data_process = 0; + $non_json_data = 0; } - return ($resource, $data, $special_data_process); + return ($resource, $data, $non_json_data); +} + +#method to take any informative input, i.e. +# - json body, +# - json part of multiform +# - request_params +sub get_info_data { + my ($self, $c) = @_; + my $ctype = $self->get_content_type($c) // ''; + my $resource = $c->request->params; + my ($resource_json,$resource_json_raw); + if ('multipart/form-data' eq $ctype) { + $resource_json_raw = delete $resource->{json}; + } elsif ('application/json' eq $ctype) { + if ($self->require_body($c)) { + $resource_json_raw = $c->stash->{body}; + } + } + if($resource_json_raw){ + $resource_json = JSON::from_json($resource_json_raw, { utf8 => 1 }); + } + { + my @common_keys = map { exists $resource->{$_} ? $_ : () }keys %$resource_json; + my (%resource_sub,%resource_json_sub); + @resource_sub{@common_keys} = @{$resource}{@common_keys}; + @resource_json_sub{@common_keys} = @{$resource_json}{@common_keys}; + if(!Compare(\%resource_sub,\%resource_json_sub)){ + return; + } + } + return {%$resource,%$resource_json}; } sub get_valid_post_data { @@ -299,11 +331,17 @@ sub forbid_link_header { return; } -sub valid_media_type { +sub get_content_type { my ($self, $c, $media_type) = @_; - my $ctype = $c->request->header('Content-Type'); $ctype =~ s/;\s+boundary.+$// if $ctype; + return $ctype; +} + +sub valid_media_type { + my ($self, $c, $media_type) = @_; + + my $ctype = $self->get_content_type($c); my $type; if(ref $media_type eq "ARRAY") { $type = join ' or ', @{ $media_type }; @@ -1002,7 +1040,14 @@ sub update_item { return $item, $form, $process_extras; } -#------ dummy & default methods +sub update_item_model{ + my($self, $c, $item, $old_resource, $resource, $form, $process_extras) = @_; + $item->update($resource); + return $item; +} +#---------------- /default methods + +#------ dummy & default accessors methods sub query_params { return [ @@ -1098,17 +1143,19 @@ sub resource_from_item{ return $res; } -sub update_item_model{ - my($self, $c, $item, $old_resource, $resource, $form, $process_extras) = @_; - $item->update($resource); - return $item; -} sub post_process_commit{ my($self, $c, $action, $item, $old_resource, $resource, $form, $process_extras) = @_; return; } +sub validate_request { + my($self, $c) = @_; + return 1; +} + +#------ /dummy & default accessors methods + sub check_transaction_control{ my($self, $c, $action, $step, %params) = @_; my $res = 1; @@ -1240,7 +1287,7 @@ sub return_representation_post{ } -sub return_csv(){ +sub return_csv{ my($self,$c) = @_; try{ my $filename = $self->check_create_csv($c); diff --git a/lib/NGCP/Panel/Role/API/PhonebookEntries.pm b/lib/NGCP/Panel/Role/API/PhonebookEntries.pm index b60f413c6c..f233d60723 100644 --- a/lib/NGCP/Panel/Role/API/PhonebookEntries.pm +++ b/lib/NGCP/Panel/Role/API/PhonebookEntries.pm @@ -25,22 +25,14 @@ sub relation{ } sub _item_rs { - my ($self, $c, $id, $resource) = @_; - my $params = $c->request->query_params; - my($owner,$type,$parameter,$value) = $self->check_owner_params($c, $params); + my ($self, $c) = @_; + my($owner,$type,$parameter,$value) = $self->check_owner_params($c); return unless $owner; my $method = 'get_'.$type.'_phonebook_rs'; my ($list_rs,$item_rs) = &$method($c, $value, $type); return $list_rs; } -sub item_by_id { - my ($self, $c, $id) = @_; - my $item_rs = $self->item_rs($c, $id); - return unless $item_rs; - return $item_rs->find($id); -} - sub get_form { my ($self, $c) = @_; my $params = $c->request->query_params; @@ -64,8 +56,22 @@ sub get_form { return; } +sub validate_request { + my($self, $c) = @_; + my $method = uc($c->request->method); + if ($method ne 'OPTIONS' && $method ne 'HEAD') { + my($owner,$type,$parameter,$value) = $self->check_owner_params($c); + return unless $owner; + } + return 1; +} + sub check_owner_params { my($self, $c, $params) = @_; + if ($c->stash->{check_owner_params}) { + return (@{$c->stash->{check_owner_params}}); + } + my @allowed_params; if ($c->user->roles eq "admin") { @allowed_params = qw/reseller_id customer_id subscriber_id/; @@ -77,15 +83,16 @@ sub check_owner_params { @allowed_params = qw/subscriber_id/; } - $params //= $c->request->params; + $params //= $self->get_info_data($c); + my %owner_params = map { $_ => $params->{$_} } grep { exists $params->{$_} } (qw/reseller_id customer_id subscriber_id/); if (!grep { exists $owner_params{$_} } @allowed_params) { - $c->log->error('"'.join('" or "', @allowed_params).'" should be specified'); - $self->error($c, HTTP_UNPROCESSABLE_ENTITY, '"'.join('" or "', @allowed_params).'" should be specified.'); + $c->log->error("'".join("' or '", @allowed_params)."' should be specified"); + $self->error($c, HTTP_UNPROCESSABLE_ENTITY, "'".join("' or '", @allowed_params)."' should be specified."); return; } @@ -93,7 +100,7 @@ sub check_owner_params { $c->log->error('Too many owners: '.join(',',keys %owner_params)); $self->error($c, HTTP_UNPROCESSABLE_ENTITY, sprintf("Only one of either %s should be specified", - join(' or ', @allowed_params))); + "'".join("' or '", @allowed_params)."'")); return; } @@ -152,8 +159,8 @@ sub check_owner_params { $self->error($c, HTTP_UNPROCESSABLE_ENTITY, "Unknown $parameter value '$value'"); return; } - - return ($owner,$type,$parameter,$value); + $c->stash->{check_owner_params} = [$owner,$type,$parameter,$value]; + return @{$c->stash->{check_owner_params}}; } sub get_reseller_phonebook_rs { diff --git a/lib/NGCP/Panel/Role/Entities.pm b/lib/NGCP/Panel/Role/Entities.pm index 811f647ab0..de9ca93320 100644 --- a/lib/NGCP/Panel/Role/Entities.pm +++ b/lib/NGCP/Panel/Role/Entities.pm @@ -20,7 +20,7 @@ sub auto :Private { if ($self->config->{log_request}) { $self->log_request($c); } - return 1; + return $self->validate_request($c); } sub end :Private { @@ -249,12 +249,11 @@ sub post { my ($c) = @_; my $guard = $self->get_transaction_control($c); { - #instead of type parameter get_form can check request method my ($form) = $self->get_form($c, 'add'); my $method_config = $self->config->{action}->{POST}; my $process_extras= {}; - my ($resource, $data, $special_data_process) = $self->get_valid_data( + my ($resource, $data, $non_json_data) = $self->get_valid_data( c => $c, method => 'POST', media_type => $method_config->{ContentType} // 'application/json', @@ -263,7 +262,7 @@ sub post { ); last unless $resource; my ($item,$data_processed_result); - if (!$special_data_process || !$data) { + if (!$non_json_data || !$data) { delete $resource->{purge_existing}; last unless $self->pre_process_form_resource($c, undef, undef, $resource, $form, $process_extras); last unless $self->validate_form( @@ -281,7 +280,7 @@ sub post { } else { try { #$processed_ok(array), $processed_failed(array), $info, $error - $data_processed_result = $self->upload_data( + $data_processed_result = $self->process_data( c => $c, data => \$data, resource => $resource, @@ -289,7 +288,7 @@ sub post { process_extras => $process_extras, ); } catch($e) { - $c->log->error("failed to upload csv: $e"); + $c->log->error("failed to proces non json data: $e"); $self->error($c, HTTP_INTERNAL_SERVER_ERROR, "Internal Server Error"); last; }; diff --git a/lib/NGCP/Panel/Role/EntitiesItem.pm b/lib/NGCP/Panel/Role/EntitiesItem.pm index 3effc50455..00f388239e 100644 --- a/lib/NGCP/Panel/Role/EntitiesItem.pm +++ b/lib/NGCP/Panel/Role/EntitiesItem.pm @@ -24,7 +24,7 @@ sub auto :Private { if($self->config->{log_request}){ $self->log_request($c); } - return 1; + return $self->validate_request($c); } sub end :Private {