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/03/22 21:33:32 UTC
[GitHub] incubator-trafficcontrol pull request #398: Org tenancy - adding tenancy to ...
GitHub user nir-sopher opened a pull request:
https://github.com/apache/incubator-trafficcontrol/pull/398
Org tenancy - adding tenancy to user
This PR adds the tenancy to the users tables.
It also adjust the API.
This PR can stand for itself. UT for the API changes were adjusted.
However, when PR-370 (user create API) is pulled merge is required, including adding tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nir-sopher/incubator-trafficcontrol org-tenancy-user-table
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafficcontrol/pull/398.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 #398
----
commit 8f77caf96866ef65bdc3a2045a8b28d9eceb7de3
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T08:39:01Z
Adding tenancy to the users table
commit b209c126b229bb1b9b9eee869b4d802a616f08f4
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T08:41:08Z
adding tenant to the user API
commit 9f3cff115ffca4d73fe4f86cee2a6ebe4af65211
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T08:43:31Z
UI: new user tenancy is derived from the creating user
commit 83920e724d7bebfa0dd887b4bcd4594d7f738506
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T08:44:26Z
A tenant cannot be deleted if a user is assigned to it
commit 5a5a0e80dd531ff46071df3154a83739a9457366
Author: nir-sopher <ni...@gmail.com>
Date: 2017-03-20T08:56:46Z
User API - adding tenancy - documentation
----
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109312063
--- Diff: traffic_ops/app/lib/UI/Utils.pm ---
@@ -42,7 +42,7 @@ use constant ADMIN => 30;
our %EXPORT_TAGS = (
'all' => [
qw(trim_whitespace is_admin is_oper is_ldap is_privileged log is_ipaddress is_ip6address is_netmask in_same_net is_hostname admin_status_id type_id type_ids
- profile_id profile_ids tm_version tm_url name_version_string is_regexp stash_role navbarpage rascal_hosts_by_cdn is_steering)
+ profile_id profile_ids tm_version tm_url name_version_string is_regexp stash_role navbarpage rascal_hosts_by_cdn is_steering defined_or_default current_user_tenant)
--- End diff --
ok, i see.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109293449
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
ldap will be going away but currently if you login as ldap you get read-only permissions on everything. so, in my opinion, find out the ID of the root tenant (select id from tenant where name = 'root') and assign that ID to the user if isLDAP. let me know if that doesn't make sense or if you disagree.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109293498
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -174,7 +179,9 @@ sub update {
registration_sent => ( $params->{registrationSent} ) ? 1 : 0,
role => $params->{role},
state_or_province => $params->{stateOrProvince},
- username => $params->{username}
+ username => $params->{username},
+ tenant_id => $tenant_id
--- End diff --
I think you can just do this to get the desired result:
tenant_id => $params->{tenantId}
and if it's not provided, it will set it to undef, otherwise it will set it accordingly
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109323632
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
Thanks:)
Indeed we plan to "seed" an active "root" tenant and potentially an admin user with this tenancy (I still need to understand how currently, without tenancy, the first admin user is added, and maybe follow the same path).
Note however that in the first phase we allow the system to work with no tenancy set and until tenancy is in use the LDAP users tenancy should be "undef".
Beyond the fact that automatically identifying the point in time a the feature is started to be in use may not be simple, I'm not comfortable with a solution in which the tenancy of the LDAP users is changed at some point in time without the operator implicitly set it. It sounds like a change in behavior only "experts" would understand.
Nevertheless, as a LDAP user has a read-only capabilities, may we consider the below solution?
current_user_tenant keeps returning "undef" for LDAP users. However, when we test what a user is allowed to do (future PR), if the user is an LDAP user he can "read" everything.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109293677
--- Diff: traffic_ops/app/lib/UI/Utils.pm ---
@@ -42,7 +42,7 @@ use constant ADMIN => 30;
our %EXPORT_TAGS = (
'all' => [
qw(trim_whitespace is_admin is_oper is_ldap is_privileged log is_ipaddress is_ip6address is_netmask in_same_net is_hostname admin_status_id type_id type_ids
- profile_id profile_ids tm_version tm_url name_version_string is_regexp stash_role navbarpage rascal_hosts_by_cdn is_steering)
+ profile_id profile_ids tm_version tm_url name_version_string is_regexp stash_role navbarpage rascal_hosts_by_cdn is_steering defined_or_default current_user_tenant)
--- End diff --
can you change this to:
defined_or_default(current_user_tenant())
otherwise, it looks like 2 properties...
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109074708
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -475,6 +489,8 @@ sub is_valid {
return $self->is_username_taken( $value, $params );
}
},
+
+ #TODO(nirs) MAYBE when tenancy is not optional, add a tenant not null check
--- End diff --
can you get rid of this? i don't think we want a bunch of "todos" in the code base with people's names 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 #398: [TC-184] Org tenancy - adding te...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafficcontrol/pull/398
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109320570
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
well, ldap will go away with amir's new authentication/authorization model so i'd rather not add another column to the tenant table that will just need to be removed.
i thought you were going to "seed" one tenant with name=root and parent=null (the "root" tenant). and this root tenant was to be the parent of all tenants...
in my opinion, for the short-term, we seed the database with the root tenant and give that tenant to "ldap users". (line 288). this will mean "ldap users" have read-only abilities on ALL resources in the system. (which is what they currently have today).
if you set their tenant=undef as you suggested, then they will have read-only role capabilities on nothing. what is the point of that? we will have a few upset users if they log in to the system via ldap and get R/O role and tenant=undef and therefore they can see nothing.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109076554
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
at some point I think ldap will be completely removed as we add the token authentication microservice but for now, i think if you login as ldap, they get the read-only role and the 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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109293693
--- Diff: traffic_ops/app/t/api/1.2/tenant.t ---
@@ -152,6 +152,12 @@ ok $t->delete_ok('/api/1.2/tenants/' . $tenantE_id)->status_is(200)->or( sub { d
ok $t->delete_ok('/api/1.2/tenants/' . $tenantD_id)->status_is(200)->or( sub { diag $t->tx->res->content->asset->{content}; } );
ok $t->delete_ok('/api/1.2/tenants/' . $tenantA_id)->status_is(200)->or( sub { diag $t->tx->res->content->asset->{content}; } );
+#TODO(nirs): move to a "tenancy" UT when written
--- End diff --
it seems like you can remove this
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109074364
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -159,6 +161,9 @@ sub update {
return $self->not_found();
}
+ #setting tenant_id to undef if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : undef;
--- End diff --
I don't think this is necessary.
tenant_id => $params->{tenantId} should be sufficient. if it's provided, great. if not, it will take on a null or undef value
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109075457
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
currently we have 2 ways to login:
1. checking ldap credentials
2. checking username/password in the tm_user table
notice that this is in the isLDAP() scenario so we give role=read-only to the user. we should also probably give tenantId = (the id of the root tenant)
does that make sense? so really, this would be fine:
"tenantId" => 1,
if we can always guarantee that the root tenant has ID=1
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109302574
--- Diff: traffic_ops/app/lib/UI/Utils.pm ---
@@ -42,7 +42,7 @@ use constant ADMIN => 30;
our %EXPORT_TAGS = (
'all' => [
qw(trim_whitespace is_admin is_oper is_ldap is_privileged log is_ipaddress is_ip6address is_netmask in_same_net is_hostname admin_status_id type_id type_ids
- profile_id profile_ids tm_version tm_url name_version_string is_regexp stash_role navbarpage rascal_hosts_by_cdn is_steering)
+ profile_id profile_ids tm_version tm_url name_version_string is_regexp stash_role navbarpage rascal_hosts_by_cdn is_steering defined_or_default current_user_tenant)
--- End diff --
I'm not sure I'm following.
defined_or_default and current_user_tenant are 2 separate utility 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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109291260
--- Diff: traffic_ops/app/db/migrations/20170315000001_user_tenancy.sql ---
@@ -0,0 +1,31 @@
+/*
--- End diff --
20170401 is on the way
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109291446
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
I'm probably missing something here. Why a person login with ldap should be "root" tenant?
Please additionally note that I cannot guarantee the ID of the root tenant. Furthermore, a root tenant is a tenant with a null parent. There is no limitation currently forcing a single 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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109312553
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
i can get this PR merged but this is the last thing that needs to change. this code will not work for ldap:
"tenantId" => $self->current_user_tenant(),
because there is no "current user" when you login as ldap. so we need to figure out what to do with tenantId in this scenario.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109293617
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -174,7 +179,9 @@ sub update {
registration_sent => ( $params->{registrationSent} ) ? 1 : 0,
role => $params->{role},
state_or_province => $params->{stateOrProvince},
- username => $params->{username}
+ username => $params->{username},
+ tenant_id => $tenant_id
--- End diff --
"There is no limitation currently forcing a single root tenant."
actually, there is, right? i mean, you can't create a tenant thru the api with parent=null. can you? i didn't think you could. i mean, technically, someone can do an insert on the tenant table directly to create multiple "root" tenants but they shouldn't be able to via the tenant api.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109302190
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -174,7 +179,9 @@ sub update {
registration_sent => ( $params->{registrationSent} ) ? 1 : 0,
role => $params->{role},
state_or_province => $params->{stateOrProvince},
- username => $params->{username}
+ username => $params->{username},
+ tenant_id => $tenant_id
--- End diff --
Currently you may be able to add a root tenant. In a pending code, where I check authorization, you will not be able to.
Note that I see a root tenant as a "base parent tenant" - it has all the capabilities on its descendant tenants, but not on tenants not descendants of its.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109313737
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
I assume the tenancy of an LDAP user should be set by the operator. He might want it to be the single tenant in the system (if he has only one tenant). He might want it to be the "root" tenant. And he might want it to be "undef" which is a powerless tenant (such like a read-only user).
If you agree with the above I think we need to provide a way to specify the "LDAP" users' tenant when the tenancy feature is first activated. Until then (and if not set) an "undef" is assumed.
What would you say about adding a property in the tenants table, allowing to mark the "LDAP" 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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109321173
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -277,6 +285,7 @@ sub current {
@data, {
"id" => "0",
"username" => $current_username,
+ "tenantId" => $self->current_user_tenant(),
--- End diff --
actually, after looking at the code closer maybe it just makes sense to leave it undef for now which is what $self->current_user_tenant() will return in this case. i'm fine with 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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109291385
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -159,6 +161,9 @@ sub update {
return $self->not_found();
}
+ #setting tenant_id to undef if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : undef;
--- End diff --
I removed the "TODO(nirs)" comment
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109291690
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -375,6 +385,10 @@ sub update_current {
if ( defined( $user->{"username"} ) ) {
$db_user->{"username"} = $user->{"username"};
}
+ if ( exists( $user->{"tenantId"} ) ) {
--- End diff --
Moved to "defined"
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109075059
--- Diff: traffic_ops/app/lib/API/Tenant.pm ---
@@ -224,6 +224,13 @@ sub delete {
return $self->alert("Tenant '$name' has children tenant(s): e.g '$existing_child'. Please update these tenants and retry.");
}
+ #The order of the below tests is intentional - allowing UT to cover all cases - TODO(nirs) remove this comment when a full "tenancy" UT is added, including permissions and such (no use in putting effort into it yet)
--- End diff --
can you remove this comment?
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109302513
--- Diff: traffic_ops/app/t/api/1.2/tenant.t ---
@@ -152,6 +152,12 @@ ok $t->delete_ok('/api/1.2/tenants/' . $tenantE_id)->status_is(200)->or( sub { d
ok $t->delete_ok('/api/1.2/tenants/' . $tenantD_id)->status_is(200)->or( sub { diag $t->tx->res->content->asset->{content}; } );
ok $t->delete_ok('/api/1.2/tenants/' . $tenantA_id)->status_is(200)->or( sub { diag $t->tx->res->content->asset->{content}; } );
+#TODO(nirs): move to a "tenancy" UT when written
--- 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.
---
[GitHub] incubator-trafficcontrol pull request #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109073415
--- Diff: traffic_ops/app/db/migrations/20170315000001_user_tenancy.sql ---
@@ -0,0 +1,31 @@
+/*
--- End diff --
can you change the name of this file? it is not longer the "latest" migration. 20170330000001_user_tenancy.sql should work.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109074671
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -375,6 +385,10 @@ sub update_current {
if ( defined( $user->{"username"} ) ) {
$db_user->{"username"} = $user->{"username"};
}
+ if ( exists( $user->{"tenantId"} ) ) {
--- End diff --
i would keep this consistent and use "defined" like it is for the other values. yes, i guess it prevents "data clearing" but maybe you shouldn't be able to wipe out your own tenant value.
---
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 #398: [TC-184] Org tenancy - adding te...
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/398#discussion_r109291378
--- Diff: traffic_ops/app/lib/API/User.pm ---
@@ -159,6 +161,9 @@ sub update {
return $self->not_found();
}
+ #setting tenant_id to undef if tenant is not set. TODO(nirs): remove when tenancy is no longer optional in the API
+ my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} : undef;
--- End diff --
Indeed it will be sufficient. Still I prefer to be explicit so the code documents itself.
Can I leave it that way?
---
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.
---