You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/04/08 20:39:25 UTC
[GitHub] [ozone] smengcl opened a new pull request, #3288: HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant
smengcl opened a new pull request, #3288:
URL: https://github.com/apache/ozone/pull/3288
## What changes were proposed in this pull request?
A permission check bug prevents non-delegated admins from assigning users to the tenant or revoking users from the tenant.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6566
## How was this patch tested?
- [ ] Pending acceptance or unit test addition
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl merged pull request #3288: HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant
Posted by GitBox <gi...@apache.org>.
smengcl merged PR #3288:
URL: https://github.com/apache/ozone/pull/3288
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #3288: HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant
Posted by GitBox <gi...@apache.org>.
smengcl commented on PR #3288:
URL: https://github.com/apache/ozone/pull/3288#issuecomment-1104261917
@errose28 No problem! Thanks for the review and comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3288: HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3288:
URL: https://github.com/apache/ozone/pull/3288#discussion_r849803987
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java:
##########
@@ -816,6 +818,22 @@ public void testTenantSetSecret() throws IOException, InterruptedException {
return null;
});
+ // Assigning/revoking user accessIds should work for a non-delegated admin
Review Comment:
I think we should split this off into its own test method. In that method, we can have a delegated and non-delegated admin. For each admin operation (add/revoke user, set secret, add/revoke admin) we try it as the delegated and non-delegated admin, and assert they pass or fail as expected. This should cover the full matrix of possible admin operations to admin type combinations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3288: HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3288:
URL: https://github.com/apache/ozone/pull/3288#discussion_r849996769
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java:
##########
@@ -816,6 +818,22 @@ public void testTenantSetSecret() throws IOException, InterruptedException {
return null;
});
+ // Assigning/revoking user accessIds should work for a non-delegated admin
Review Comment:
I agree. Some of the JUnit test methods in this class has grown too large. Should probably do a test refactoring at some point (at the cost of slightly longer integration test run time due to the extra init/cleanup, but should be worth it).
Moved the test to a **new** JUnit test method for assigning/revoking tenant accessIds (users) as non-delegated admin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3288: HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3288:
URL: https://github.com/apache/ozone/pull/3288#discussion_r850009865
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java:
##########
@@ -816,6 +818,22 @@ public void testTenantSetSecret() throws IOException, InterruptedException {
return null;
});
+ // Assigning/revoking user accessIds should work for a non-delegated admin
Review Comment:
I have made it a new method `testTenantAdminOperations`. Should be better now.
I'm keeping `testTenantSetSecret` separate as it is testing some edge cases (e.g. set empty secret) that we don't really care in `testTenantAdminOperations`, albeit overlapping slightly with the test addition here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org