[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/incubator-trafficcontrol/pull/753


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[nir-sopher]

Github user nir-sopher commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/753#discussion_r131254521
  
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -376,6 +387,10 @@ sub get_deliveryservices_not_assigned_to_user {
 
my $rs_links = $self->db->resultset("Deliveryservice")->search( undef, 
{ order_by => "xml_id" } );
while ( my $row = $rs_links->next ) {
+   if 
(!$tenant_utils->is_ds_resource_accessible_to_tenant($tenants_data, 
$row->tenant_id, $user->tenant_id)) {
--- End diff --

OK, this is the last fix missing. On 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[nir-sopher]

Github user nir-sopher commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/753#discussion_r131245830
  
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -376,6 +387,10 @@ sub get_deliveryservices_not_assigned_to_user {
 
my $rs_links = $self->db->resultset("Deliveryservice")->search( undef, 
{ order_by => "xml_id" } );
while ( my $row = $rs_links->next ) {
+   if 
(!$tenant_utils->is_ds_resource_accessible_to_tenant($tenants_data, 
$row->tenant_id, $user->tenant_id)) {
--- End diff --

@mitchell852 
We do not check the tenancy of the current user here.
Do you want to add 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[mitchell852]

Github user mitchell852 commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/753#discussion_r129939021
  
--- Diff: traffic_ops/app/lib/API/Deliveryservice.pm ---
@@ -949,6 +959,8 @@ sub get_deliveryservices_by_userId {
my @data;
if ( defined($deliveryservices) ) {
while ( my $row = $deliveryservices->next ) {
+   ### we do not check here if we have access to the DS.i
--- End diff --

i still think it's important to check tenancy on both the user and the ds's 
returned. remember, no guarantee that the user using this api has a high-level 
role. i think there's no harm in ALWAYS limiting ds's to those in your tenant 
tree. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[mitchell852]

Github user mitchell852 commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/753#discussion_r129940638
  
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -408,16 +423,24 @@ sub assign_deliveryservices {
if ( !defined($user) ) {
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_user_resource_accessible($tenants_data, 
$user->tenant_id)) {
+   #no access to resource tenant
+   return $self->alert("Invalid user. This user is not available 
to you for assignment.");
+   }
 
if ( $replace ) {
# start fresh and delete existing user/deliveryservice 
associations
+   # We are not checking DS tenancy on deletion - we manage the 
user here - we remove permissions to touch a DS
my $delete = 
$self->db->resultset('DeliveryserviceTmuser')->search( { tm_user_id => $user_id 
} );
$delete->delete();
}
 
my @values = ( [ qw( deliveryservice tm_user_id ) ]); # column names 
are required for 'populate' function
 
foreach my $ds_id (@{ $delivery_services }) {
+   #not checking ds tenancy - this is a user operation, setting 
his premissions, not a "DS" operation
--- End diff --

in my opinion we SHOULD be checking tenancy here to ensure only ds's inside 
the current users tenant tree are assigned, however, that is hard to do with 
the way this code is implemented plus since assigning individual ds's to a user 
is going to be replace with tenancy, i would not worry about this but i'd 
remove the comments about not checking 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[mitchell852]

Github user mitchell852 commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/753#discussion_r129937967
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceUser.pm ---
@@ -31,6 +32,18 @@ sub delete {
 return $self->forbidden();
 }
 
+my $user = $self->db->resultset('TmUser')->find( { id => $user_id } );
+if ( !defined($user) ) {
+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_user_resource_accessible($tenants_data, 
$user->tenant_id)) {
+#no access to resource tenant
+return $self->forbidden();
--- End diff --

can you add forbidden messages?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[mitchell852]

Github user mitchell852 commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/753#discussion_r129937865
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceUser.pm ---
@@ -31,6 +32,18 @@ sub delete {
 return $self->forbidden();
 }
 
+my $user = $self->db->resultset('TmUser')->find( { id => $user_id } );
--- End diff --

i feel like there is no harm in checking tenancy on both the user and the 
DS.

if i am trying to remove DS1 from user1 first check that user1 has a tenant 
inside my tenant tree and then check that DS1 has a tenant in my tenant tree.

if DS1 is not inside my tenant tree, i should know NOTHING about that DS. 
to me, that DS does not exist.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[mitchell852]

Github user mitchell852 commented on a diff in the pull request:


https://github.com/apache/incubator-trafficcontrol/pull/753#discussion_r129940738
  
--- Diff: traffic_ops/app/lib/Utils/Tenant.pm ---
@@ -224,6 +224,15 @@ sub is_ds_resource_accessible {
 return $self->_is_resource_accessable( $tenants_data, 
$resource_tenancy);
 }
 
+sub is_ds_resource_accessible_to_tenant {
+my $self = shift;
+my $tenants_data = shift;
+my $resource_tenancy = shift;
+my $user_tenancy = shift;
+
+return $self->_is_resource_accessable_to_tenant( $tenants_data, 
$resource_tenancy, $user_tenancy);
+}
+
 sub ignore_ds_users_table {
--- End diff --

ignore_ds_users_table() is going away, right?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...

[nir-sopher]

GitHub user nir-sopher opened a pull request:

https://github.com/apache/incubator-trafficcontrol/pull/753

[TC-463] User tenancy ds assignment



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/nir-sopher/incubator-trafficcontrol 
user-tenancy-ds-assignment

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-trafficcontrol/pull/753.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 #753


commit 99a8dbd0fab50b6aec8d861d0781f01f20461b35
Author: nir-sopher 
Date:   2017-06-21T01:09:00Z

DS/User table - user tenancy verification

commit c65ffe5c27e2f9ccecc6989672609776d95daaa7
Author: nir-sopher 
Date:   2017-06-26T15:27:24Z

DS tenancy - almost no effect on DS/Tmuser operation

commit 89db4545afdcd9440b8aee8e8acf198536004a33
Author: nir-sopher 
Date:   2017-06-28T03:04:18Z

Moving to UI::TenantUtils to Utils::Tenant, and changeing assign_user 
tenancy check error to 400




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---