You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by nir-sopher <gi...@git.apache.org> on 2017/07/25 19:35:47 UTC
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
GitHub user nir-sopher opened a pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752
[TC-460] Ds tenancy validation server
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nir-sopher/incubator-trafficcontrol ds-tenancy-validation-server
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafficcontrol/pull/752.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #752
----
commit 941641a880888bc504b2f3012433de86629a43fc
Author: nir-sopher <ni...@qwilt.com>
Date: 2017-07-04T15:32:41Z
DS/Server tenancy check (DS based) - not tested
commit c14f17c386abf0bdf865d19b4141091c3ca874f8
Author: nir-sopher <ni...@qwilt.com>
Date: 2017-07-05T08:52:34Z
DS/Server tenancy - tests
commit cfa4104ea5af12dda1c8fb98896f99ba99dea32e
Author: nir-sopher <ni...@qwilt.com>
Date: 2017-07-05T14:16:06Z
DS/Server tenancy checks - additional endpoints
commit fb62c8cb5e416a2cdb58bd071d5c98ea0e9e04b2
Author: nir-sopher <ni...@qwilt.com>
Date: 2017-07-20T13:29:41Z
DS/Server tenancy checks - UT fixes after rebase
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130410587
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -726,41 +746,26 @@ sub get_servers_by_type {
my $orderby = $self->param('orderby') || "hostName";
my $orderby_snakecase = lcfirst( decamelize($orderby) );
+ my $forbidden;
--- End diff --
k
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129933537
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -569,10 +575,17 @@ sub get_unassigned_servers_by_dsid {
$ds_server_criteria{'deliveryservice.id'} = $ds_id;
my @assigned_servers;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->forbidden();
+ }
+ elsif ( &is_privileged($self) || $tenant_utils->ignore_ds_users_table() || $self->is_delivery_service_assigned($ds_id) ) {
@assigned_servers = $self->db->resultset('DeliveryserviceServer')->search( \%ds_server_criteria, { prefetch => [ 'deliveryservice', 'server' ] } )->get_column('server')->all();
}
else {
+ #for the reviewer - I believe it should turn into forbidden as well
--- End diff --
yes, this is wrong. should be $self->forbidden
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238784
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -651,7 +664,14 @@ sub get_eligible_servers_by_dsid {
my %ds_server_criteria;
$ds_server_criteria{'deliveryservice.id'} = $ds_id;
- if ( !&is_privileged($self) && !$self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->forbidden();
+ }
+ elsif ( !&is_privileged($self) && !$tenant_utils->ignore_ds_users_table() && !$self->is_delivery_service_assigned($ds_id) ) {
+ #for the reviewer - I believe it should turn into forbidden as well
return $self->alert("Forbidden. Delivery service not assigned to user.");
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130407547
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -80,15 +86,21 @@ sub assign_servers_to_ds {
return $self->forbidden();
}
- if ( ref($servers) ne 'ARRAY' ) {
- return $self->alert("Servers must be an array");
- }
-
my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id } );
if ( !defined($ds) ) {
return $self->not_found();
}
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->alert("Invalid delivery-service assignment. The delivery-service is not avaialble for your tenant.");
--- End diff --
ok, i see your point
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafficcontrol/pull/752
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238728
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -445,18 +431,27 @@ sub get_servers_by_dsid {
my $orderby_snakecase = lcfirst( decamelize($orderby) );
my $helper = new Utils::Helper( { mojo => $self } );
+ my $ds = $self->db->resultset('Deliveryservice')->search( { 'me.id' => $ds_id }, { prefetch => ['type'] } )->single();
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
my @ds_servers;
my $forbidden;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $servers;
+
+ if (defined($ds) && !$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ $forbidden = "Forbidden. Delivery service not available for user's tenant.";
+ return ($forbidden, $servers);
+ }
+ elsif ( &is_privileged($self) || $tenant_utils->ignore_ds_users_table() || $self->is_delivery_service_assigned($ds_id) ) {
--- End diff --
Same, answer:)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238531
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -80,15 +86,21 @@ sub assign_servers_to_ds {
return $self->forbidden();
}
- if ( ref($servers) ne 'ARRAY' ) {
- return $self->alert("Servers must be an array");
- }
-
my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id } );
if ( !defined($ds) ) {
return $self->not_found();
}
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->alert("Invalid delivery-service assignment. The delivery-service is not avaialble for your tenant.");
--- End diff --
Are you sure?
This is not part of the path
$r->post("/api/$version/deliveryserviceserver")->over( authenticated => 1, not_ldap => 1 )->to( 'DeliveryServiceServer#assign_servers_to_ds', namespace => $namespace );
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130410539
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -569,10 +575,17 @@ sub get_unassigned_servers_by_dsid {
$ds_server_criteria{'deliveryservice.id'} = $ds_id;
my @assigned_servers;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
--- End diff --
ok, sounds good. thx for verifying
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129913397
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -27,6 +28,11 @@ sub index {
my @data;
my $orderby = $self->param('orderby') || "deliveryservice";
+ #FOR THE REVIEWER - Currently I do not check DS tenancy here.
--- End diff --
in my opinion tenancy is ALWAYs checked. it doesn't hurt, does it? it's always good imo to ensure the user (regardless of role) is only seeing the appropriate delivery services...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129931600
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -489,11 +484,22 @@ sub get_edge_servers_by_dsid {
my $self = shift;
my $ds_id = $self->param('id');
+ my $ds = $self->db->resultset('Deliveryservice')->search( { 'me.id' => $ds_id } )->single();
+ if ( !defined($ds) ) {
+ return $self->not_found();
+ }
+
my $ds_servers;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->forbidden();
--- End diff --
can you add a forbidden message?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129928084
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -27,6 +28,11 @@ sub index {
my @data;
my $orderby = $self->param('orderby') || "deliveryservice";
+ #FOR THE REVIEWER - Currently I do not check DS tenancy here.
--- End diff --
As I saw it, the ds-admin should not be aware of specific servers existence, so this API should not be available to him.
However, as you see usecases a ds-admin may approach this API, I'll had the tenancy check
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129934310
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -1021,6 +1030,10 @@ sub details {
}
my $rs_ds_data = $row->deliveryservice_servers;
+ #FOR THE REVIEWER - Currently I do not check DS tenancy here.
--- End diff --
same thing. always check tenancy for anything ds-related imo
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130239129
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -569,10 +575,17 @@ sub get_unassigned_servers_by_dsid {
$ds_server_criteria{'deliveryservice.id'} = $ds_id;
my @assigned_servers;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
--- End diff --
Seems ok to me.
case 1: "use=tenancy" == true, and resource is accessible to tenant:
1. "if" test is false, continue
2. "elseif" test is true (as ignore_ds_users_table==true), so we get the list of servers
case 2: "use=tenancy" == true, and resource is not accessible to tenant:
1. "if" test is true, return forbidden
case 3: "use=tenancy" == false, and ds is assigned to user
1. "if" test is false, continue
2. "elseif" test is true (as the ds is assigned to user), so we get the list of servers
case 3: "use=tenancy" == false, and ds is not assigned to user
1. "if" test is false, continue
2. "elseif" test is false (as the ds is not assigned to user),
3. We reach the failure
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129912516
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -27,6 +28,11 @@ sub index {
my @data;
my $orderby = $self->param('orderby') || "deliveryservice";
+ #FOR THE REVIEWER - Currently I do not check DS tenancy here.
--- End diff --
With the new roles/capabilities model there will be a lot of flexibility regarding roles and capabilities. For example, we could create a role called ds-admin with the following capabilities:
- ds-read
- ds-write
- ds-server-read
- ds-server-write
- ds-regex-read
- etc
so that means anyone with this ds-admin role can manage all aspects of a ds, including changing the server assignments.
other companies may only want to attach ds-server-write to the a higher level role but we can't control that so my suggestion is we always check tenancy and filter results accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129934238
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -947,6 +952,10 @@ sub details_v11 {
}
my $rs_ds_data = $row->deliveryservice_servers;
+ #FOR THE REVIEWER - Currently I do not check DS tenancy here.
--- End diff --
always check tenancy in my opinion if use_tenancy=1. never rely on roles / capabilities because roles can be setup to include whatever capabilities the system owner wants. there is no harm in always ensuring the user has the proper tenancy. they can always sidestep tenancy by assigning themself with the root tenant...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129933717
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -651,7 +664,14 @@ sub get_eligible_servers_by_dsid {
my %ds_server_criteria;
$ds_server_criteria{'deliveryservice.id'} = $ds_id;
- if ( !&is_privileged($self) && !$self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->forbidden();
+ }
+ elsif ( !&is_privileged($self) && !$tenant_utils->ignore_ds_users_table() && !$self->is_delivery_service_assigned($ds_id) ) {
+ #for the reviewer - I believe it should turn into forbidden as well
return $self->alert("Forbidden. Delivery service not assigned to user.");
--- End diff --
yep, forbidden
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238653
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -400,40 +401,25 @@ sub get_servers_by_status {
my $orderby = $self->param('orderby') || "hostName";
my $orderby_snakecase = lcfirst( decamelize($orderby) );
+ my $forbidden;
--- End diff --
The function was previously testing the ds_user table as well and filtering by it.
We agreed it should be removed and replaced only with "is-oper" test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129933611
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -569,10 +575,17 @@ sub get_unassigned_servers_by_dsid {
$ds_server_criteria{'deliveryservice.id'} = $ds_id;
my @assigned_servers;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
--- End diff --
you might want to rethink this if/elseif/else seems wrong.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129931534
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -445,18 +431,27 @@ sub get_servers_by_dsid {
my $orderby_snakecase = lcfirst( decamelize($orderby) );
my $helper = new Utils::Helper( { mojo => $self } );
+ my $ds = $self->db->resultset('Deliveryservice')->search( { 'me.id' => $ds_id }, { prefetch => ['type'] } )->single();
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
my @ds_servers;
my $forbidden;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $servers;
+
+ if (defined($ds) && !$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ $forbidden = "Forbidden. Delivery service not available for user's tenant.";
+ return ($forbidden, $servers);
+ }
+ elsif ( &is_privileged($self) || $tenant_utils->ignore_ds_users_table() || $self->is_delivery_service_assigned($ds_id) ) {
--- End diff --
same thing. is this going to change to $tenant_utils->use_tenancy()
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129913844
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -27,6 +28,11 @@ sub index {
my @data;
my $orderby = $self->param('orderby') || "deliveryservice";
+ #FOR THE REVIEWER - Currently I do not check DS tenancy here.
--- End diff --
if this operation is of a cdn owner for debug then the cdn owner needs to have the root tenant and there will be no fear of hidden data.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238734
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -489,11 +484,22 @@ sub get_edge_servers_by_dsid {
my $self = shift;
my $ds_id = $self->param('id');
+ my $ds = $self->db->resultset('Deliveryservice')->search( { 'me.id' => $ds_id } )->single();
+ if ( !defined($ds) ) {
+ return $self->not_found();
+ }
+
my $ds_servers;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->forbidden();
--- End diff --
Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130407783
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -223,8 +240,19 @@ sub remove_server_from_ds {
my $ds_id = $self->param('dsId');
my $server_id = $self->param('serverId');
- if ( !&is_privileged($self) && !$self->is_delivery_service_assigned($ds_id) ) {
- $self->forbidden("Forbidden. Delivery service not assigned to user.");
+ my $tenant_utils = Utils::Tenant->new($self);
+ if ( !&is_privileged($self) && !$tenant_utils->ignore_ds_users_table() && !$self->is_delivery_service_assigned($ds_id) ) {
--- End diff --
yep, i like the idea of maybe having 1 function use_tenancy() or whatever that keys off of the use-tenancy parameter
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238561
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -223,8 +240,19 @@ sub remove_server_from_ds {
my $ds_id = $self->param('dsId');
my $server_id = $self->param('serverId');
- if ( !&is_privileged($self) && !$self->is_delivery_service_assigned($ds_id) ) {
- $self->forbidden("Forbidden. Delivery service not assigned to user.");
+ my $tenant_utils = Utils::Tenant->new($self);
+ if ( !&is_privileged($self) && !$tenant_utils->ignore_ds_users_table() && !$self->is_delivery_service_assigned($ds_id) ) {
--- End diff --
Probably yes, we need to decide if we merge only the 2 parameters (use-tenancy & ignore_ds_user_assignment) or the functions as well.
If we merge also the fucntions, lets do it in one "search replace" commit
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130240112
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -1021,6 +1030,10 @@ sub details {
}
my $rs_ds_data = $row->deliveryservice_servers;
+ #FOR THE REVIEWER - Currently I do not check DS tenancy here.
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129931292
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -400,40 +401,25 @@ sub get_servers_by_status {
my $orderby = $self->param('orderby') || "hostName";
my $orderby_snakecase = lcfirst( decamelize($orderby) );
+ my $forbidden;
--- End diff --
not sure why this method needed to change (get_servers_by_status) as it seems unrelated to delivery services.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130240113
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -947,6 +952,10 @@ sub details_v11 {
}
my $rs_ds_data = $row->deliveryservice_servers;
+ #FOR THE REVIEWER - Currently I do not check DS tenancy here.
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129930025
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -223,8 +240,19 @@ sub remove_server_from_ds {
my $ds_id = $self->param('dsId');
my $server_id = $self->param('serverId');
- if ( !&is_privileged($self) && !$self->is_delivery_service_assigned($ds_id) ) {
- $self->forbidden("Forbidden. Delivery service not assigned to user.");
+ my $tenant_utils = Utils::Tenant->new($self);
+ if ( !&is_privileged($self) && !$tenant_utils->ignore_ds_users_table() && !$self->is_delivery_service_assigned($ds_id) ) {
--- End diff --
wasn't ignore_ds_users_table going to go away? so this would become
if ( !&is_privileged($self) && !$tenant_utils->use_tenancy() && !$self->is_delivery_service_assigned($ds_id) ) {
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238764
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -569,10 +575,17 @@ sub get_unassigned_servers_by_dsid {
$ds_server_criteria{'deliveryservice.id'} = $ds_id;
my @assigned_servers;
- if ( &is_privileged($self) || $self->is_delivery_service_assigned($ds_id) ) {
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->forbidden();
+ }
+ elsif ( &is_privileged($self) || $tenant_utils->ignore_ds_users_table() || $self->is_delivery_service_assigned($ds_id) ) {
@assigned_servers = $self->db->resultset('DeliveryserviceServer')->search( \%ds_server_criteria, { prefetch => [ 'deliveryservice', 'server' ] } )->get_column('server')->all();
}
else {
+ #for the reviewer - I believe it should turn into forbidden as well
--- End diff --
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129933912
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -726,41 +746,26 @@ sub get_servers_by_type {
my $orderby = $self->param('orderby') || "hostName";
my $orderby_snakecase = lcfirst( decamelize($orderby) );
+ my $forbidden;
--- End diff --
not sure why this changed. seems unrelated to delivery services but if you are trying to establish consistency, i am fine with it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238813
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -726,41 +746,26 @@ sub get_servers_by_type {
my $orderby = $self->param('orderby') || "hostName";
my $orderby_snakecase = lcfirst( decamelize($orderby) );
+ my $forbidden;
--- End diff --
This function previously filtered records by the ds_user table (only servers used for the DSes viewable to the user)
We agreed on mail to remove the test and put "is-oper"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r129914745
--- Diff: traffic_ops/app/lib/API/DeliveryServiceServer.pm ---
@@ -80,15 +86,21 @@ sub assign_servers_to_ds {
return $self->forbidden();
}
- if ( ref($servers) ne 'ARRAY' ) {
- return $self->alert("Servers must be an array");
- }
-
my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id } );
if ( !defined($ds) ) {
return $self->not_found();
}
+ my $tenant_utils = Utils::Tenant->new($self);
+ my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+ if (!$tenant_utils->is_ds_resource_accessible($tenants_data, $ds->tenant_id)) {
+ return $self->alert("Invalid delivery-service assignment. The delivery-service is not avaialble for your tenant.");
--- End diff --
this could be a forbidden
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130238709
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -400,40 +401,25 @@ sub get_servers_by_status {
my $orderby = $self->param('orderby') || "hostName";
my $orderby_snakecase = lcfirst( decamelize($orderby) );
+ my $forbidden;
--- End diff --
This function tested the "ds_user" table and filtered DSes accordingly.
We discussed it (by mail) and decided to remove this test and add only a "is-oper" test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #752: [TC-460] Ds tenancy validation s...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on a diff in the pull request:
https://github.com/apache/incubator-trafficcontrol/pull/752#discussion_r130407887
--- Diff: traffic_ops/app/lib/API/Server.pm ---
@@ -400,40 +401,25 @@ sub get_servers_by_status {
my $orderby = $self->param('orderby') || "hostName";
my $orderby_snakecase = lcfirst( decamelize($orderby) );
+ my $forbidden;
--- End diff --
ok
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---