[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...

[asfgit]

Github user asfgit closed the pull request at:

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


---
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 #751: [TC-462] Ds tenancy validation r...

[nir-sopher]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130981656
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -222,6 +262,18 @@ sub delete {
return $self->forbidden();
}
 
+   my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id 
} );
+   if ( !defined($ds) ) {
+   #allow deletion if the ds is not valid
--- 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130975670
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -222,6 +262,18 @@ sub delete {
return $self->forbidden();
}
 
+   my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id 
} );
+   if ( !defined($ds) ) {
+   #allow deletion if the ds is not valid
--- End diff --

can  you make this change? this doesn't make sense to me. if the ds doesn't 
exist, don't worry about deleting ds regexes...just throw a not_found()


---
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 #751: [TC-462] Ds tenancy validation r...

[nir-sopher]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130425792
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceMatches.pm ---
@@ -28,11 +29,19 @@ sub index {
my $format = $self->param("format") || "";
 
my $rs;
-   if ( _privileged($self) ) {
+   # TO the reviewer: Do we need to override the "is_priviledged" here byt 
the standard "ignore_ds_user_table" flag?
+   # What is the reason of the is_priv test - was someone just dussmissed 
the ds_tmuser table tests
+   if ( _privileged($self)) {
--- End diff --

Check removed and UT adjusted


---
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 #751: [TC-462] Ds tenancy validation r...

[nir-sopher]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130420173
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -27,12 +28,18 @@ use Validate::Tiny ':all';
 sub all {
my $self = shift;
 
+   my $tenant_utils = Utils::Tenant->new($self);
+   my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
my $rs;
-   if ( _privileged($self) ) {
+   if ( _privileged($self) or $tenant_utils->ignore_ds_users_table()) {
--- End diff --

OK.
Done in the relevant PR (which is pending the parameters fix PR)
I suggest this PR will be pulled and we will fix for all commits in the 
relevant PRs


---
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 #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130406581
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -222,6 +262,18 @@ sub delete {
return $self->forbidden();
}
 
+   my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id 
} );
+   if ( !defined($ds) ) {
+   #allow deletion if the ds is not valid
--- End diff --

i don't think you can have an entry in deliveryservice_regex without the 
same delivery service in the deliveryservice table due to the foreign key in 
deliveryservice_regex. 

if that happens then you have a data integrity problem and someone will 
have to clean up the database manually and whatever bug is allowing this to 
happen needs to be fixed. So i would just stick with the basic:

my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id } );
if ( !defined($ds) ) {
return $self->not_found();
}


---
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 #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130405550
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -27,12 +28,18 @@ use Validate::Tiny ':all';
 sub all {
my $self = shift;
 
+   my $tenant_utils = Utils::Tenant->new($self);
+   my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
my $rs;
-   if ( _privileged($self) ) {
+   if ( _privileged($self) or $tenant_utils->ignore_ds_users_table()) {
--- End diff --

i think that would make sense to merge the 2 functions


---
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 #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130405449
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceMatches.pm ---
@@ -28,11 +29,19 @@ sub index {
my $format = $self->param("format") || "";
 
my $rs;
-   if ( _privileged($self) ) {
+   # TO the reviewer: Do we need to override the "is_priviledged" here byt 
the standard "ignore_ds_user_table" flag?
+   # What is the reason of the is_priv test - was someone just dussmissed 
the ds_tmuser table tests
+   if ( _privileged($self)) {
--- End diff --

no idea why it was restricted to "is privileged" which is really the same 
is "is operations or above" since ldap only users don't work any more


---
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 #751: [TC-462] Ds tenancy validation r...

[nir-sopher]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130237388
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -222,6 +262,18 @@ sub delete {
return $self->forbidden();
}
 
+   my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id 
} );
+   if ( !defined($ds) ) {
+   #allow deletion if the ds is not valid
--- End diff --

If sue to some bug or inconsitency - the a regex exists but the parent DS 
does not, we cannot check tenancy.
However we want to allow the deletion of the regex - which for itself 
exists.


---
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 #751: [TC-462] Ds tenancy validation r...

[nir-sopher]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130237244
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -66,6 +73,12 @@ sub index {
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->forbidden();
--- End diff --

yes, this code was written before we discussed this issue. Will fix in all 
3 PRs


---
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 #751: [TC-462] Ds tenancy validation r...

[nir-sopher]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130237223
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -27,12 +28,18 @@ use Validate::Tiny ':all';
 sub all {
my $self = shift;
 
+   my $tenant_utils = Utils::Tenant->new($self);
+   my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
my $rs;
-   if ( _privileged($self) ) {
+   if ( _privileged($self) or $tenant_utils->ignore_ds_users_table()) {
--- End diff --

This code was written earlier...
BTW, I thought you were just concerned about having 2 separate parameters. 
Would you like to merge the "use-tenancy" and "ignore-ds-user-table" functions 
as well?


---
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 #751: [TC-462] Ds tenancy validation r...

[nir-sopher]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r130237222
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceMatches.pm ---
@@ -28,11 +29,19 @@ sub index {
my $format = $self->param("format") || "";
 
my $rs;
-   if ( _privileged($self) ) {
+   # TO the reviewer: Do we need to override the "is_priviledged" here byt 
the standard "ignore_ds_user_table" flag?
+   # What is the reason of the is_priv test - was someone just dussmissed 
the ds_tmuser table tests
+   if ( _privileged($self)) {
--- End diff --

I was trying to keep the functionality as is, and just add the tenancy 
tests.
Do you know why was it restricted to a privileged user in the first place?


---
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 #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r129904376
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceMatches.pm ---
@@ -28,11 +29,19 @@ sub index {
my $format = $self->param("format") || "";
 
my $rs;
-   if ( _privileged($self) ) {
+   # TO the reviewer: Do we need to override the "is_priviledged" here byt 
the standard "ignore_ds_user_table" flag?
+   # What is the reason of the is_priv test - was someone just dussmissed 
the ds_tmuser table tests
+   if ( _privileged($self)) {
--- End diff --

what about if you change this line to:

if ( (use-tenancy=1) || _privileged($self)) {

^^ that's just pseudo code obviously

if tenancy is in place, fine let them in and you will only see "matches" 
for the ds's assigned to you
if tenancy is not in place, then it will fall back to what it was before 
and will check is_privileged



---
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 #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r129904886
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -27,12 +28,18 @@ use Validate::Tiny ':all';
 sub all {
my $self = shift;
 
+   my $tenant_utils = Utils::Tenant->new($self);
+   my $tenants_data = $tenant_utils->create_tenants_data_from_db();
+
my $rs;
-   if ( _privileged($self) ) {
+   if ( _privileged($self) or $tenant_utils->ignore_ds_users_table()) {
--- End diff --

I thought you were changing this to check the 'use-tenancy' parameter so i 
would expect something like this to be:

if ( _privileged($self) or $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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r129906932
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -222,6 +262,18 @@ sub delete {
return $self->forbidden();
}
 
+   my $ds = $self->db->resultset('Deliveryservice')->find( { id => $ds_id 
} );
+   if ( !defined($ds) ) {
+   #allow deletion if the ds is not valid
--- End diff --

I dont' understand this part. why not just return 404 not found if ds is 
invalid?


---
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 #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r129905512
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -66,6 +73,12 @@ sub index {
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->forbidden();
--- End diff --

can you put a message in here like return $self->forbidden('this delivery 
service belongs to a tenant you are not authorized to see'); ... or something 
like that


---
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 #751: [TC-462] Ds tenancy validation r...

[mitchell852]

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


https://github.com/apache/incubator-trafficcontrol/pull/751#discussion_r129906584
  
--- Diff: traffic_ops/app/lib/API/DeliveryServiceRegexes.pm ---
@@ -66,6 +73,12 @@ sub index {
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->forbidden();
--- End diff --

actually, can you do that on all your forbidden message for now on that are 
the result of a tenancy check 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...

[nir-sopher]

GitHub user nir-sopher opened a pull request:

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

[TC-462] Ds tenancy validation regex



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

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

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

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


commit bf90642b77c22c9db78bbc80115fe11753d16374
Author: nir-sopher 
Date:   2017-06-26T10:51:39Z

DS tenancy validation - regexes API

commit 33779ade730a3597ee33f6bb12b7e41a1bf192d2
Author: nir-sopher 
Date:   2017-07-04T15:33:46Z

Tenancy check - deliver-service-match




---
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.
---