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:32:03 UTC
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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 <ni...@qwilt.com>
Date: 2017-06-26T10:51:39Z
DS tenancy validation - regexes API
commit 33779ade730a3597ee33f6bb12b7e41a1bf192d2
Author: nir-sopher <ni...@qwilt.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 ( &is_privileged($self) ) {
+ if ( &is_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 ( &is_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 ( &is_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 ( &is_privileged($self) ) {
+ if ( &is_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 ( &is_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 ( &is_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 ( &is_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 ( &is_privileged($self) ) {
+ if ( &is_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 ( &is_privileged($self) ) {
+ if ( &is_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 ( &is_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 ( &is_privileged($self)) {
--- End diff --
what about if you change this line to:
if ( (use-tenancy=1) || &is_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 ( &is_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 ( &is_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafficcontrol pull request #751: [TC-462] Ds tenancy validation r...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---