You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/10/06 16:04:41 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request, #3001: WIP - Address TODOs in PermissionsIT
DomGarguilo opened a new pull request, #3001:
URL: https://github.com/apache/accumulo/pull/3001
This PR attempts to address and remove the TODOs in PermissionsIT
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3001: Remove TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#issuecomment-1284572990
> @ctubbsii - any opinion on this?
The advantage of doing this is that it clears the TODO from the code, so we know we've triaged it. If it stays as a TODO, then somebody might create another ticket later. So, let's merge this.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3001: WIP - Address TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#issuecomment-1279240703
> > @DomGarguilo - is the plan to still try and fix this for 2.1.0? I may have time this weekend to look at it if you don't have a chance, not sure yet.
>
> This kind of fell off my radar. I won't be working on it this weekend so feel free to take a look +1
Sounds good, I will be doing some testing and documentation stuff but if I have some extra time I will look at this too otherwise just leave it for you. I figured I'd ask to make sure didn't have something already in progress.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #3001: Remove TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #3001:
URL: https://github.com/apache/accumulo/pull/3001
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #3001: Remove TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#issuecomment-1281282773
@cshannon I opted to just replace the TODO comments and open up https://github.com/apache/accumulo/issues/3027 in their place. Let me know if you think this is fine or if I should remove the whole if statements and comments all together.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3001: WIP - Address TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#discussion_r990133359
##########
test/src/main/java/org/apache/accumulo/test/functional/PermissionsIT.java:
##########
@@ -419,7 +421,14 @@ private void testMissingSystemPermission(String tableNamePrefix, AccumuloClient
break;
case OBTAIN_DELEGATION_TOKEN:
if (saslEnabled()) {
Review Comment:
It doesn't appear that SASL is actually enabled in this test so this code never executes which I'm sure is why the TODO is there in the first place and why the test seems to work.
SASL is enabled by setting static system properties that are checked in `AccumuloClusterHarness` so there may be some work here to only enable it for that one check or maybe it's simplest to create a new test class (or one that extends this) that sets the property to enable it so the `TestingKdc` is initialized and this can be tested.
##########
test/src/main/java/org/apache/accumulo/test/functional/PermissionsIT.java:
##########
@@ -592,7 +601,16 @@ private void testGrantedSystemPermission(String tableNamePrefix, AccumuloClient
break;
case OBTAIN_DELEGATION_TOKEN:
if (saslEnabled()) {
Review Comment:
Same comment as above
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #3001: WIP - Address TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#issuecomment-1279233341
> @DomGarguilo - is the plan to still try and fix this for 2.1.0? I may have time this weekend to look at it if you don't have a chance, not sure yet.
This kind of fell off my radar. I won't be working on it this weekend so feel free to take a look :+1:
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #3001: WIP - Address TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#issuecomment-1270338186
I marked this as WIP because there is behavior that seem incorrect to me. In the following code that I added here:
```java
// assert the test user does not have permissions to obtain a delegation token
loginAs(rootUser);
assertFalse(root_client.securityOperations().hasSystemPermission(testUser.getPrincipal(),
SystemPermission.OBTAIN_DELEGATION_TOKEN));
// login as the test user and attempt to obtain a delegation token
loginAs(testUser);
DelegationToken token =
test_user_client.securityOperations().getDelegationToken(new DelegationTokenConfig());
```
It checks that the `testUser` does not have the permissions to obtain a delegation token, then successfully obtains a delegation token. Unless I am misunderstanding something, this does not seem like it should be happening. Can anyone confirm or deny this?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3001: Remove TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#issuecomment-1281495012
> @cshannon I opted to just replace the TODO comments and open up #3027 in their place. Let me know if you think this is fine or if I should remove the whole if statements and comments all together.
I'm ok either leaving the comment and if statement to fill in later or just removing it as it's just a test class. The new updated comment here is still basically a TODO (without calling it TODO) so not sure it's any better than before. To me the best way to track it is the new issue which you just created so I'm not sure we need to leave it and maybe can just remove the entire empty if statement block. @ctubbsii - any opinion on this?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3001: WIP - Address TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#issuecomment-1279184378
@DomGarguilo - is the plan to still try and fix this for 2.1.0? I may have time this weekend to look at it if you don't have a chance, not sure yet.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3001: WIP - Address TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#issuecomment-1279779093
@DomGarguilo - I looked at this for about an hour today and tried to get the test working but I had issues getting Kerberos to work properly with the clients/server, etc as I haven't used it much before. There is a good amount of configuration and setup here to get everything working even with all the helper utilities in the parent class. I don't have anymore time to look at it so you or someone else will have to come back to it later but this is obviously not super high priority and can be done when someone has more time to fix it.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3001: WIP - Address TODOs in PermissionsIT
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #3001:
URL: https://github.com/apache/accumulo/pull/3001#discussion_r990233686
##########
test/src/main/java/org/apache/accumulo/test/functional/PermissionsIT.java:
##########
@@ -419,7 +421,14 @@ private void testMissingSystemPermission(String tableNamePrefix, AccumuloClient
break;
case OBTAIN_DELEGATION_TOKEN:
if (saslEnabled()) {
Review Comment:
Oops, good catch, guess I missed that.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org