Tenant utils #644

nir-sopher · 2017-06-05T19:34:50Z

Adding the "TenantUtils" class allowing to check if resource is accessible (tenancy wise).
The class provides further tenants related functionality like get the tenants in hierarchic order, check if a tenant is a root tenant, etc.

The tenants API was adjusted to use this class as well as permit "tenant resource" operations based on the current user tenancy

mitchell852

some comments for you. seems to work as i'd expect.

mitchell852 · 2017-06-28T16:27:15Z

traffic_ops/app/lib/API/Tenant.pm

-	while ( my $row = $rs_data->next ) {
-		$idnames{ $row->id } = $row->name;
-	}
+	my $tenant_utils = UI::TenantUtils->new($self);


this doesn't seem like it's in the right place. how about putting TenantUtils in the Utils directory?

and then you can just call it Utils::Tenant

Cool:) Will do.
I was not aware of this dir, just put it next to the "Util.pm"....

mitchell852 · 2017-06-28T17:01:14Z

traffic_ops/app/lib/API/Tenant.pm

-	if ( !defined( $params->{parentId}) && !$self->isRootTenant($id) ) {
+	my $tenant_utils = UI::TenantUtils->new($self);
+	my $tenants_data = $tenant_utils->create_tenants_data_from_db(undef);
+


how about right here adding something like:

if ( is_root_tenant($tenants_data, $id) ) {
return $self->alert("Root tenant cannot be updated.");
}

I think we agreed that the root tenant can't be updated but I could be wrong. Anyhow, if you put this code right about here, then you won't have to check is_root_tenant() anymore in this function which I think will make your code easier to read.

Indeed we agreed that a "root" tenant cannot become not root, and a non-root cannot become root. It also cannot be disabled.
Does it mean that a root tenant cannot be changed? I;m not sure but it indeed simolify things.
Will do

yes, i would say the root tenant can't be changed in any way. you can't change it's name, or it's active flag or it's parent (which has to be null). that is a record in the database that can't be changed at all thru the api.

mitchell852 · 2017-06-28T18:36:55Z

traffic_ops/app/lib/API/Tenant.pm

+	#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();
+	if (!defined($current_resource_tenancy)) {
+		#no parent - the tenant is its-own owner


every tenant must have a parent, right? except for the root tenant. therefore, I don't think this is necessary if you follow my advice above and exit the function if the tenant is the root tenant.

mitchell852 · 2017-06-28T18:44:22Z

traffic_ops/app/lib/API/Tenant.pm

+			return $self->alert("Failed to retrieve tenant height.");
+		}
+
+		if ($parent_depth+$tenant_height+1 > $tenant_utils->max_heirarchy_limit())


can we just get rid of the max_heirarchy_limit? that would get rid of your need to figure out depth and height and simplify this code

The limit is there to avoid infinite loops in tenant-utils in case of cyclic parent pointing. There is no way I;m aware of to create such loops but bugs may prove me wrong:)
As I set a limit in the loops themselves, I wanted to keep the code coherent and block the option to create deeper hierarchy.

so i've been thinking about this a little more. by putting in a max hierarchy depth, you are putting in a safety net in case somehow cyclical parents are created. for example.

root has no parent
foo has baz parent <-- cyclical
bar has foo parent
baz has bar parent

however, this feels pretty hacky putting in this safety net and it could cover up valid problems. i think it's more important to just ensure that the parent for a tenant can never be assigned as itself or as a child tenant and then make sure you have a robust set of tests to validate this. for example:

root
-- foo
--- bar
---- baz

when updating the bar tenant, bar and baz are invalid parents.

IF a bug slips thru and an infinite loop is created, then we fix it and write a test case to represent the scenario that created an infinite loop to ensure it is fixed and doesn't happen again. safety nets worry me because they may cover up bugs that need to be fixed.

inside your is_ancestor function, you probably need a recursive function to walk down the tree starting with the tenant to be updated to ensure that the new parent is != self or child.

so your function call would look like this

if (!is_ancestor($tenant, $parent)) return $self->alert("Invalid parent")

here's an article regarding recursion in perl: https://perlmaven.com/recursive-subroutines

Indeed, safety nets may cover up bugs, but not in this case - the operation will fail instead of crashing or getting stuck. Then we can debug it.
Furthermore, we will have an error in the log pointing out the reason of the failure - what we will not have if we get into an infinite loop/recursion.

mitchell852 · 2017-06-28T18:46:39Z

traffic_ops/app/lib/API/Tenant.pm

+		my $is_tenant_achestor_of_parent = $tenant_utils->is_anchestor_of($tenants_data, $id, $params->{parentId});
+		if (!defined($is_tenant_achestor_of_parent))
+		{
+			return $self->alert("Failed to check tenant and parent current relations.");


I don't understand this error message

The above test comes to verify we do not create cyclic graph by parent pointing. We check if the designated child is already an ancestor of his parent.

mitchell852 · 2017-06-28T18:50:31Z

traffic_ops/app/lib/API/Tenant.pm

+		return $self->alert("Failed to retrieve parent tenant depth.");
+	}
+
+	if ($parent_depth+1 > $tenant_utils->max_heirarchy_limit()-1)


again, if we get rid of the max_heirarchy_limit, then you can simplify this code.

mitchell852 · 2017-06-28T18:53:11Z

traffic_ops/app/lib/API/Tenant.pm

+	my $tenant_utils = UI::TenantUtils->new($self);
+	my $tenants_data = $tenant_utils->create_tenants_data_from_db(undef);
+
+	if (!$tenant_utils->is_tenant_resource_accessible($tenants_data, $parent_tenant)) {


i don't think that deleting a tenant should matter what the parent tenant is. for example, if this is the tenant hierarcy:

root
-- tenant 1
--- tenant 1a
--- tenant 1b

and i have a user with tenant = tenant 1a, then i should be able to delete tenant 1a even though i don't have tenant 1 assigned to me. what do you think?

I see the parent as an owner of the child, and as such he is the one to be able to delete it.
It is a matter of taste, but note: anyhow a user must not delete his own tenant...

oh, that's a good point. if i try to delete my own tenant, i will get back "sorry, this tenant is assigned to a user", right?

mitchell852 · 2017-06-28T19:05:10Z

traffic_ops/app/lib/API/User.pm

@@ -404,7 +406,7 @@ sub current {
 	my $self = shift;
 	my @data;
 	my $current_username = $self->current_user()->{username};
-
+	my $tenantUtils = UI::TenantUtils->new($self);


this can go away. you are not using this variable.

nir-sopher · 2017-06-29T03:16:04Z

True. Note that there is also a matter of consistency: Tenant update can also be done only by the parent. Would you like to change that as well?

…

On Jun 29, 2017 12:23 AM, "Jeremy Mitchell" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In traffic_ops/app/lib/API/Tenant.pm <#644 (comment)> : > @@ -248,7 +326,17 @@ sub delete { if ( !defined($tenant) ) { return $self->not_found(); } - my $name = $self->db->resultset('Tenant')->search( { id => $id } )->get_column('name')->single(); + + my $parent_tenant = $tenant->parent_id; + + my $tenant_utils = UI::TenantUtils->new($self); + my $tenants_data = $tenant_utils->create_tenants_data_from_db(undef); + + if (!$tenant_utils->is_tenant_resource_accessible($tenants_data, $parent_tenant)) { oh, that's a good point. if i try to delete my own tenant, i will get back "sorry, this tenant is assigned to a user", right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#644 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXGkYJ98rZeJkzjhKMlVyTgHkyJFR3Prks5sIsRjgaJpZM4NwdcT> .

nir-sopher · 2017-06-29T11:17:05Z

file move moved as required

…o the API for simple testing

…ancy checks

mitchell852 requested changes Jun 28, 2017

View reviewed changes

nir-sopher and others added 24 commits July 11, 2017 12:55

Order tenants list in reply in heirarchic order

2de6d9c

Org-Tenancy Fixing error string

1fda2f7

Tenancy utils - is_root_tenant

bf79652

Org tenancy - removing redundant DB call

a910eca

Tenant-utils - current_user_tenant

817c86d

TenantUtils - tenant resource tenancy test

b6df0f2

Tenant list - order by

b40776f

tenancy utils - get_tenants_list and get_tenant

e3a3274

tenantutils - becoming stateless

523f58e

tenant utils - depth, height and relations. including temp addition t…

a1cd8a3

…o the API for simple testing

tenancy checks - return 403

46a116c

Tenant utils - testing capabilities

bcda683

Tenant utils - documentations and changes

ef12049

tenant utils - perltidy

0398392

Tenant utils - rebase fixes

49991bb

tenant utils - no special treatment for ldap

e883a83

Tenant util - remove readable/writeable separation

6167bf4

Tenant resource tenancy check - style change

e7c6ad3

Tenancy access verification - global disable mechanisem

73f7b6e

Tenant-utils - minor func seperation

d72eb9c

Tenant-utils PR review changes

5ead721

Moving UI::TenantUtils to Utils::Tenant

ec34b8c

Tenant is the resource owbner of itself + move some 403 to 400 on ten…

a4d2f77

…ancy checks

Moving DS creation to use tenant-utils to get current user tenant

f310303

asfgit closed this in d37e386 Jul 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tenant utils #644

Tenant utils #644

nir-sopher commented Jun 5, 2017

mitchell852 left a comment

mitchell852 Jun 28, 2017

mitchell852 Jun 28, 2017

nir-sopher Jun 28, 2017

mitchell852 Jun 28, 2017

nir-sopher Jun 28, 2017

mitchell852 Jun 28, 2017

mitchell852 Jun 28, 2017

nir-sopher Jun 28, 2017

mitchell852 Jun 28, 2017

nir-sopher Jun 28, 2017

mitchell852 Jun 28, 2017

mitchell852 Jun 29, 2017

mitchell852 Jun 29, 2017

nir-sopher Jun 29, 2017

mitchell852 Jun 28, 2017

nir-sopher Jun 28, 2017

mitchell852 Jun 28, 2017

mitchell852 Jun 28, 2017

nir-sopher Jun 28, 2017

mitchell852 Jun 28, 2017

mitchell852 Jun 28, 2017

nir-sopher commented Jun 29, 2017 via email

nir-sopher commented Jun 29, 2017

Tenant utils #644

Tenant utils #644

Conversation

nir-sopher commented Jun 5, 2017

mitchell852 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nir-sopher commented Jun 29, 2017 via email

nir-sopher commented Jun 29, 2017