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:50:06 UTC
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
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 <ni...@qwilt.com>
Date: 2017-06-21T01:09:00Z
DS/User table - user tenancy verification
commit c65ffe5c27e2f9ccecc6989672609776d95daaa7
Author: nir-sopher <ni...@qwilt.com>
Date: 2017-06-26T15:27:24Z
DS tenancy - almost no effect on DS/Tmuser operation
commit 89db4545afdcd9440b8aee8e8acf198536004a33
Author: nir-sopher <ni...@qwilt.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
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/753#discussion_r131253207
--- 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 --
when assigning ds's to a user, the ds's should be limited to those in the current user's tenant tree, right? so yes, i think you should be checking the tenancy of the current user.
---
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 #753: [TC-463] User tenancy ds assignm...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #753: [TC-463] User tenancy ds assignm...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---