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/04/06 08:24:12 UTC
[GitHub] incubator-trafficcontrol pull request #433: Tenancy validation tenant
GitHub user nir-sopher opened a pull request:
https://github.com/apache/incubator-trafficcontrol/pull/433
Tenancy validation tenant
WIP.
First step in tenancy verification and tests - treating "tenant" as a resource and verify who can write/read it.
WIP as this was tested via UT only partially. Further tests are pending PR 370 (user create API).
However there is no real harm in pulling this PR as it does not touch old functional paths.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nir-sopher/incubator-trafficcontrol tenancy-validation-tenant
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafficcontrol/pull/433.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 #433
----
commit 01a63012ac0e48eddb20ad44a8ee63ef280db944
Author: nir-sopher <ni...@gmail.com>
Date: 2017-04-06T08:19:56Z
Org tenancy - test tenancy utility
commit 3ac706e237c8f6199fa8c5b7d15e8eb2d9168b89
Author: nir-sopher <ni...@gmail.com>
Date: 2017-04-06T08:20:22Z
Org tenancy - test tenancy of resource '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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r110311677
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -44,14 +44,16 @@ sub index {
my $orderby = $self->param('orderby') || "name";
my $rs_data = $self->db->resultset("Tenant")->search( undef, {order_by => 'me.' . $orderby } );
--- End diff --
maybe you can just make this query smarter like
my $rs_data = $self->db->resultset("Tenant")->search( {-in (my-list-of-tenants)}, {order_by => 'me.' . $orderby } );
^^ I can't remember the dbix syntax for that off the top of my head but basically a dbix query that looks like:
select * from tenant where id in (a list of the user's tenant plus it's children)
---
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 #433: [TC-184] Tenancy validation for ...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher closed the pull request at:
https://github.com/apache/incubator-trafficcontrol/pull/433
---
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 issue #433: [TC-184] Tenancy validation for resourc...
Posted by nir-sopher <gi...@git.apache.org>.
Github user nir-sopher commented on the issue:
https://github.com/apache/incubator-trafficcontrol/pull/433
Following a discussion with @mitchell852 I'm closing this PR.
New PR will be provided, supplying an API that supports tenants table caching to reduce calls to DB when the API is used for a few times in the same scope
---
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 issue #433: [TC-184] Tenancy validation for resourc...
Posted by mitchell852 <gi...@git.apache.org>.
Github user mitchell852 commented on the issue:
https://github.com/apache/incubator-trafficcontrol/pull/433
make sure you rebase this when you get a chance. you have conflicts.
---
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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r112028749
--- Diff: traffic_ops/app/lib/UI/Utils.pm ---
@@ -413,4 +414,81 @@ sub current_user_tenant {
return $self->db->resultset('TmUser')->search( { username => $self->current_user()->{username} } )->get_column('tenant_id')->single();
}
+sub verify_tenancy {
--- End diff --
I would suggest that this method is never called inside a loop. for example, when looping thru a query result set. the 2 queries performed in this method will cause problems when TO is setup with a remote postgres database.
---
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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r112025226
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -117,14 +123,25 @@ sub update {
my $is_active = $params->{active};
if ( !$params->{active} && $self->isRootTenant($id)) {
- return $self->alert("Root user cannot be in-active.");
+ return $self->alert("Root tenant cannot be in-active.");
}
-
- if ( !defined($params->{parentId}) && !isRootTenant($id) ) {
- return $self->alert("Only the \"root\" tenant can have no parent.");
+ #this is a write operation, allowed only by parents of the tenant (which are the owners of the resource of type tenant)
+ my $current_resource_tenancy = $self->db->resultset('Tenant')->search( { id => $id } )->get_column('parent_id')->single();
--- End diff --
also, if you make root tenant uneditable, you can get rid of this complexity. and simply do
if (!verify_tenancy_for_write($self, $params->{parentId})) {
return $self->alert("Parent tenant to be set is not under user's 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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r112023530
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -44,14 +44,16 @@ sub index {
my $orderby = $self->param('orderby') || "name";
my $rs_data = $self->db->resultset("Tenant")->search( undef, {order_by => 'me.' . $orderby } );
while ( my $row = $rs_data->next ) {
- push(
- @data, {
- "id" => $row->id,
- "name" => $row->name,
- "active" => \$row->active,
- "parentId" => $row->parent_id,
- }
- );
+ if (verify_tenancy_for_read($self, $row->id)) {
--- End diff --
this might cause problems. for example, if there are 200 tenants, i think this will run 400 queries. since it looks like verify_tenancy() makes 2 queries for each time it is called. i would recommend making your initial query smarter.
---
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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r110648569
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -44,14 +44,16 @@ sub index {
my $orderby = $self->param('orderby') || "name";
my $rs_data = $self->db->resultset("Tenant")->search( undef, {order_by => 'me.' . $orderby } );
--- End diff --
I assume you mean that I'll go down the tenancy tree instead of testing the permissions tenant by tenant. If I'm correct, you practically assume that the "parent/child" is the only relationship between tenants we have, which may not be the case in the future.
---
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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r112024097
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -117,14 +123,25 @@ sub update {
my $is_active = $params->{active};
if ( !$params->{active} && $self->isRootTenant($id)) {
--- End diff --
can we just make root tenant uneditable? for example:
if $self->isRootTenant($id) return $self->alert("Cannot edit 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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r112024314
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -117,14 +123,25 @@ sub update {
my $is_active = $params->{active};
if ( !$params->{active} && $self->isRootTenant($id)) {
--- End diff --
and if you make root tenant uneditable, then just make parentId a required field...
---
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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r110311804
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -44,14 +44,16 @@ sub index {
my $orderby = $self->param('orderby') || "name";
my $rs_data = $self->db->resultset("Tenant")->search( undef, {order_by => 'me.' . $orderby } );
--- End diff --
here's an example where a query is limited by "in"
https://github.com/apache/incubator-trafficcontrol/blob/master/traffic_ops/app/lib/API/Cdn.pm#L701
---
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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r110311384
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -162,11 +179,17 @@ sub create {
return $self->alert("Tenant name is required.");
}
+ #not allowing to create additional root tenants.
+ #there is no real problem with that, but no real use also
my $parent_id = $params->{parentId};
if ( !defined($parent_id) ) {
return $self->alert("Parent Id is required.");
}
+ if (!verify_tenancy_for_write($self, $params->{parentId})) {
--- End diff --
I don't understand why you need a verify_tenancy_for_read and verify_tenancy_for_write
I thought if you got this far thru amir's new authentication/authorization model, then you'd simply do a tenant test. for example, if i was trying to do:
PUT /api/tenant/5
i would first have to get thru the new AAA model and then there would be a check that my user has tenant_id=5 (or a parent of 5) like so
if (!hasTenant(5)) { return $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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r110647853
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -162,11 +179,17 @@ sub create {
return $self->alert("Tenant name is required.");
}
+ #not allowing to create additional root tenants.
+ #there is no real problem with that, but no real use also
my $parent_id = $params->{parentId};
if ( !defined($parent_id) ) {
return $self->alert("Parent Id is required.");
}
+ if (!verify_tenancy_for_write($self, $params->{parentId})) {
--- End diff --
The read/write separation came to ease the move to more complicated relationships between tenants in the future.
What I really aimed to is a read/write function per tenant anchor:
verify_tenancy_for_tenant_read/verify_tenancy_for_tenant_write
verify_tenancy_for_user_read/verify_tenancy_for_user_write
verify_tenancy_for_ds_read/verify_tenancy_for_ds_write
verify_tenancy_for_cdn_read/verify_tenancy_for_cdn_write
and maybe more (e.g. verify_tenancy_for_profile_read/verify_tenancy_for_profile_write).
The need of such an interface is the support of more complicated tenants relationships. Most cases are indeed indirectly covered by the AAA but not all.
For example, the ISP admin has the ability to manage parameters within a profile. The CP admin also has this capability, as a profile can be now set to a DS.
If now I want to allow the users of ISP1 tenant to view profiles of CP1 tenant, but not to edit them, I need to distinguish read and write on the tenancy level.
The functional API I'm adding now is far from being complete. Much more should be considered when deciding on the final one. For now I only separate read&write to simplify further changes.
Additionally, this separation has an added value, allowing LDAP users to view all data but not write to 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 #433: [TC-184] Tenancy validation for ...
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/433#discussion_r112025733
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -107,6 +111,8 @@ sub update {
}
--- End diff --
right under line 95, can you just do a check to see if they are allowed to edit this tenant?
if (!verify_tenancy_for_write($self, $params->{id})) {
return $self->alert("This tenant is not under user's 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.
---